[v4,9/9] clk: fixed-factor: Let clk framework find parent
diff mbox series

Message ID 20190412183150.102131-10-sboyd@kernel.org
State New, archived
Headers show
Series
  • Rewrite clk parent handling
Related show

Commit Message

Stephen Boyd April 12, 2019, 6:31 p.m. UTC
Convert this driver to a more modern way of specifying parents now that
we have a way to specify clk parents by DT index. This lets us nicely
avoid a problem where a parent clk name isn't know because the parent
clk hasn't been registered yet.

Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk-fixed-factor.c | 53 +++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 20 deletions(-)

Comments

Stephen Boyd April 19, 2019, 10:13 p.m. UTC | #1
Quoting Stephen Boyd (2019-04-12 11:31:50)
> Convert this driver to a more modern way of specifying parents now that
> we have a way to specify clk parents by DT index. This lets us nicely
> avoid a problem where a parent clk name isn't know because the parent
> clk hasn't been registered yet.
> 
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

Applied to clk-next
Guenter Roeck April 23, 2019, 6:09 p.m. UTC | #2
Hi,

On Fri, Apr 12, 2019 at 11:31:50AM -0700, Stephen Boyd wrote:
> Convert this driver to a more modern way of specifying parents now that
> we have a way to specify clk parents by DT index. This lets us nicely
> avoid a problem where a parent clk name isn't know because the parent
> clk hasn't been registered yet.
> 
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>

This patch causes a substantial number of crashes of qemu boot tests in -next.

Failed tests: 
	arm:versatilepb:versatile_defconfig:aeabi:pci:scsi:mem128:versatile-pb:rootfs 
	arm:versatilepb:versatile_defconfig:aeabi:pci:mem128:versatile-pb:initrd 
	arm:versatileab:versatile_defconfig:mem128:versatile-ab:initrd 
	arm:beagle:multi_v7_defconfig:sd:mem256:omap3-beagle:rootfs 
	arm:beaglexm:multi_v7_defconfig:sd:mem512:omap3-beagle-xm:rootfs 
	arm:overo:multi_v7_defconfig:sd:mem256:omap3-overo-tobi:rootfs 
	arm:realview-pb-a8:realview_defconfig:realview_pb:mem512:arm-realview-pba8:initrd 
	arm:realview-pbx-a9:realview_defconfig:realview_pb:arm-realview-pbx-a9:initrd 
	arm:realview-eb:realview_defconfig:realview_eb:mem512:arm-realview-eb:initrd 
	arm:realview-eb-mpcore:realview_defconfig:realview_eb:mem512:arm-realview-eb-11mp-ctrevb:initrd 
	arm:integratorcp:integrator_defconfig:mem128:integratorcp:initrd 
	arm:mps2-an385:mps2_defconfig:mps2-an385:initrd

Most of the time the crash happens too early to generate a log,
but here is one:

[    0.000000] [<2100bd59>] (unwind_backtrace) from [<2100b11f>] (show_stack+0xb/0xc)
[    0.000000] [<2100b11f>] (show_stack) from [<211b2d27>] (Ldiv0_64+0x9/0x1a)
[    0.000000] [<211b2d27>] (Ldiv0_64) from [<21038e87>] (clocks_calc_max_nsecs+0x1d/0x62)
[    0.000000] [<21038e87>] (clocks_calc_max_nsecs) from [<21038fb1>] (__clocksource_update_freq_scale+0xe5/0x11c)
[    0.000000] [<21038fb1>] (__clocksource_update_freq_scale) from [<21038ff1>] (__clocksource_register_scale+0x9/0x40)
[    0.000000] [<21038ff1>] (__clocksource_register_scale) from [<212a8713>] (mps2_timer_init+0xaf/0x29c)
[    0.000000] [<212a8713>] (mps2_timer_init) from [<212a85b1>] (timer_probe+0x49/0x80)
[    0.000000] [<212a85b1>] (timer_probe) from [<2129d639>] (start_kernel+0x1c5/0x2f4)
[    0.000000] [<2129d639>] (start_kernel) from [<00000000>] (  (null))
[    0.000000] clocksource: mps2-clksrc: mask: 0xffffffff max_cycles: 0x0, max_idle_ns: 0 ns
[    0.000000] Division by zero in kernel.

Reverting the crash fixes the problem. Bisect log attached.

Guenter

---
# bad: [76c938fcaa4b4a5d8f05fa907925d5043834964e] Add linux-next specific files for 20190423
# good: [085b7755808aa11f78ab9377257e1dad2e6fa4bb] Linux 5.1-rc6
git bisect start 'HEAD' 'v5.1-rc6'
# bad: [ed04f675fa2c22316d7b57bea1258a18a47537ea] Merge remote-tracking branch 'crypto/master'
git bisect bad ed04f675fa2c22316d7b57bea1258a18a47537ea
# bad: [f66d30ddc658fb37848e8e6297b1e658fa297e79] Merge remote-tracking branch 'hid/for-next'
git bisect bad f66d30ddc658fb37848e8e6297b1e658fa297e79
# good: [24523334fd0feef03f3dc42487c158c233455676] Merge remote-tracking branch 'tegra/for-next'
git bisect good 24523334fd0feef03f3dc42487c158c233455676
# bad: [4d5d5f95d0ef4ba470287a941d06600889760ab7] Merge remote-tracking branch 'btrfs-kdave/for-next'
git bisect bad 4d5d5f95d0ef4ba470287a941d06600889760ab7
# bad: [c8040e3c8ab0870b3dfa502cc931258fc04709c6] Merge remote-tracking branch 's390/features'
git bisect bad c8040e3c8ab0870b3dfa502cc931258fc04709c6
# bad: [4209fc3374cfa572aa2defb8ecafe94a9db3c7d4] Merge remote-tracking branch 'csky/linux-next'
git bisect bad 4209fc3374cfa572aa2defb8ecafe94a9db3c7d4
# good: [21eb35a1ae4db08d32e2b5a8d9fe476c16056511] Merge commit 'tags/clk-fixes-for-linus^0' into clk-next
git bisect good 21eb35a1ae4db08d32e2b5a8d9fe476c16056511
# good: [3f644cdb2351fe21cded6ee1e5c13ea7905c3a64] Merge branch 'clk-zynq' into clk-next
git bisect good 3f644cdb2351fe21cded6ee1e5c13ea7905c3a64
# bad: [0db9597d81d918605d4d36c87ab140228fe14150] Merge remote-tracking branch 'clk-samsung/for-next'
git bisect bad 0db9597d81d918605d4d36c87ab140228fe14150
# bad: [e04cb6e358cbcdce56cda317725131252ecf6ccd] Merge branch 'clk-parent-rewrite-1' into clk-next
git bisect bad e04cb6e358cbcdce56cda317725131252ecf6ccd
# good: [89a5ddcc799d5d7dbcf6197b79dafc1dc9f997f5] clk: Add of_clk_hw_register() API for early clk drivers
git bisect good 89a5ddcc799d5d7dbcf6197b79dafc1dc9f997f5
# good: [dde4eff47c82c52a72af333d9e55370eee6d95d6] clk: Look for parents with clkdev based clk_lookups
git bisect good dde4eff47c82c52a72af333d9e55370eee6d95d6
# bad: [ecbf3f1795fda56122632c1d024cfd0d3f4fe353] clk: fixed-factor: Let clk framework find parent
git bisect bad ecbf3f1795fda56122632c1d024cfd0d3f4fe353
# good: [601b6e93304a65f8f7c37168763ab9ba5b195ce5] clk: Allow parents to be specified via clkspec index
git bisect good 601b6e93304a65f8f7c37168763ab9ba5b195ce5
# first bad commit: [ecbf3f1795fda56122632c1d024cfd0d3f4fe353] clk: fixed-factor: Let clk framework find parent
Stephen Boyd April 23, 2019, 6:22 p.m. UTC | #3
Quoting Guenter Roeck (2019-04-23 11:09:22)
> Hi,
> 
> On Fri, Apr 12, 2019 at 11:31:50AM -0700, Stephen Boyd wrote:
> > Convert this driver to a more modern way of specifying parents now that
> > we have a way to specify clk parents by DT index. This lets us nicely
> > avoid a problem where a parent clk name isn't know because the parent
> > clk hasn't been registered yet.
> > 
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> > Cc: Chen-Yu Tsai <wens@csie.org>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> 
> This patch causes a substantial number of crashes of qemu boot tests in -next.
> 
> Failed tests: 
>         arm:versatilepb:versatile_defconfig:aeabi:pci:scsi:mem128:versatile-pb:rootfs 
>         arm:versatilepb:versatile_defconfig:aeabi:pci:mem128:versatile-pb:initrd 
>         arm:versatileab:versatile_defconfig:mem128:versatile-ab:initrd 
>         arm:beagle:multi_v7_defconfig:sd:mem256:omap3-beagle:rootfs 
>         arm:beaglexm:multi_v7_defconfig:sd:mem512:omap3-beagle-xm:rootfs 
>         arm:overo:multi_v7_defconfig:sd:mem256:omap3-overo-tobi:rootfs 
>         arm:realview-pb-a8:realview_defconfig:realview_pb:mem512:arm-realview-pba8:initrd 
>         arm:realview-pbx-a9:realview_defconfig:realview_pb:arm-realview-pbx-a9:initrd 
>         arm:realview-eb:realview_defconfig:realview_eb:mem512:arm-realview-eb:initrd 
>         arm:realview-eb-mpcore:realview_defconfig:realview_eb:mem512:arm-realview-eb-11mp-ctrevb:initrd 
>         arm:integratorcp:integrator_defconfig:mem128:integratorcp:initrd 
>         arm:mps2-an385:mps2_defconfig:mps2-an385:initrd
> 
> Most of the time the crash happens too early to generate a log,
> but here is one:
> 
> [    0.000000] [<2100bd59>] (unwind_backtrace) from [<2100b11f>] (show_stack+0xb/0xc)
> [    0.000000] [<2100b11f>] (show_stack) from [<211b2d27>] (Ldiv0_64+0x9/0x1a)
> [    0.000000] [<211b2d27>] (Ldiv0_64) from [<21038e87>] (clocks_calc_max_nsecs+0x1d/0x62)
> [    0.000000] [<21038e87>] (clocks_calc_max_nsecs) from [<21038fb1>] (__clocksource_update_freq_scale+0xe5/0x11c)
> [    0.000000] [<21038fb1>] (__clocksource_update_freq_scale) from [<21038ff1>] (__clocksource_register_scale+0x9/0x40)
> [    0.000000] [<21038ff1>] (__clocksource_register_scale) from [<212a8713>] (mps2_timer_init+0xaf/0x29c)
> [    0.000000] [<212a8713>] (mps2_timer_init) from [<212a85b1>] (timer_probe+0x49/0x80)
> [    0.000000] [<212a85b1>] (timer_probe) from [<2129d639>] (start_kernel+0x1c5/0x2f4)
> [    0.000000] [<2129d639>] (start_kernel) from [<00000000>] (  (null))
> [    0.000000] clocksource: mps2-clksrc: mask: 0xffffffff max_cycles: 0x0, max_idle_ns: 0 ns
> [    0.000000] Division by zero in kernel.
> 
> Reverting the crash fixes the problem. Bisect log attached.
> 

Thanks for the report. This was bisected yesterday by kernelci.org (see
https://lkml.kernel.org/r/5cbe596c.1c69fb81.e252.b9d0@mx.google.com for
more details). Can you try the latest version of clk-next and see if it
fixes the early crashes? The one-liner patch I attached in that thread
should be all you need.

It would be even better for kernelci to find the offending patch like
you've done here and reply to the patch on the mailing list.

Finally, can you share your qemu recipe? I can pull it into my testing
and integration workflow so that this doesn't happen again.
Guenter Roeck April 23, 2019, 7:26 p.m. UTC | #4
On Tue, Apr 23, 2019 at 11:22:57AM -0700, Stephen Boyd wrote:
> Quoting Guenter Roeck (2019-04-23 11:09:22)
> > Hi,
> > 
> > On Fri, Apr 12, 2019 at 11:31:50AM -0700, Stephen Boyd wrote:
> > > Convert this driver to a more modern way of specifying parents now that
> > > we have a way to specify clk parents by DT index. This lets us nicely
> > > avoid a problem where a parent clk name isn't know because the parent
> > > clk hasn't been registered yet.
> > > 
> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: Jeffrey Hugo <jhugo@codeaurora.org>
> > > Cc: Chen-Yu Tsai <wens@csie.org>
> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > 
> > This patch causes a substantial number of crashes of qemu boot tests in -next.
> > 
> > Failed tests: 
> >         arm:versatilepb:versatile_defconfig:aeabi:pci:scsi:mem128:versatile-pb:rootfs 
> >         arm:versatilepb:versatile_defconfig:aeabi:pci:mem128:versatile-pb:initrd 
> >         arm:versatileab:versatile_defconfig:mem128:versatile-ab:initrd 
> >         arm:beagle:multi_v7_defconfig:sd:mem256:omap3-beagle:rootfs 
> >         arm:beaglexm:multi_v7_defconfig:sd:mem512:omap3-beagle-xm:rootfs 
> >         arm:overo:multi_v7_defconfig:sd:mem256:omap3-overo-tobi:rootfs 
> >         arm:realview-pb-a8:realview_defconfig:realview_pb:mem512:arm-realview-pba8:initrd 
> >         arm:realview-pbx-a9:realview_defconfig:realview_pb:arm-realview-pbx-a9:initrd 
> >         arm:realview-eb:realview_defconfig:realview_eb:mem512:arm-realview-eb:initrd 
> >         arm:realview-eb-mpcore:realview_defconfig:realview_eb:mem512:arm-realview-eb-11mp-ctrevb:initrd 
> >         arm:integratorcp:integrator_defconfig:mem128:integratorcp:initrd 
> >         arm:mps2-an385:mps2_defconfig:mps2-an385:initrd
> > 
> > Most of the time the crash happens too early to generate a log,
> > but here is one:
> > 
> > [    0.000000] [<2100bd59>] (unwind_backtrace) from [<2100b11f>] (show_stack+0xb/0xc)
> > [    0.000000] [<2100b11f>] (show_stack) from [<211b2d27>] (Ldiv0_64+0x9/0x1a)
> > [    0.000000] [<211b2d27>] (Ldiv0_64) from [<21038e87>] (clocks_calc_max_nsecs+0x1d/0x62)
> > [    0.000000] [<21038e87>] (clocks_calc_max_nsecs) from [<21038fb1>] (__clocksource_update_freq_scale+0xe5/0x11c)
> > [    0.000000] [<21038fb1>] (__clocksource_update_freq_scale) from [<21038ff1>] (__clocksource_register_scale+0x9/0x40)
> > [    0.000000] [<21038ff1>] (__clocksource_register_scale) from [<212a8713>] (mps2_timer_init+0xaf/0x29c)
> > [    0.000000] [<212a8713>] (mps2_timer_init) from [<212a85b1>] (timer_probe+0x49/0x80)
> > [    0.000000] [<212a85b1>] (timer_probe) from [<2129d639>] (start_kernel+0x1c5/0x2f4)
> > [    0.000000] [<2129d639>] (start_kernel) from [<00000000>] (  (null))
> > [    0.000000] clocksource: mps2-clksrc: mask: 0xffffffff max_cycles: 0x0, max_idle_ns: 0 ns
> > [    0.000000] Division by zero in kernel.
> > 
> > Reverting the crash fixes the problem. Bisect log attached.
> > 
> 
> Thanks for the report. This was bisected yesterday by kernelci.org (see
> https://lkml.kernel.org/r/5cbe596c.1c69fb81.e252.b9d0@mx.google.com for
> more details). Can you try the latest version of clk-next and see if it
> fixes the early crashes? The one-liner patch I attached in that thread
> should be all you need.
> 
Yes, it done. Thanks for taking care of it.

> It would be even better for kernelci to find the offending patch like
> you've done here and reply to the patch on the mailing list.
> 
> Finally, can you share your qemu recipe? I can pull it into my testing
> and integration workflow so that this doesn't happen again.
> 

It is all available at https://github.com/groeck/linux-build-test/.
You would need an out-of-tree version of qemu to test mps2-an385, though;
upstream qemu rejected the patches necessary to make it work for testing
linux kernel images.

Guenter

Patch
diff mbox series

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index 241b3f8c61a9..67cc7e515e42 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -64,12 +64,14 @@  const struct clk_ops clk_fixed_factor_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_fixed_factor_ops);
 
-struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
-		const char *name, const char *parent_name, unsigned long flags,
-		unsigned int mult, unsigned int div)
+static struct clk_hw *
+__clk_hw_register_fixed_factor(struct device *dev, struct device_node *np,
+		const char *name, const char *parent_name, int index,
+		unsigned long flags, unsigned int mult, unsigned int div)
 {
 	struct clk_fixed_factor *fix;
 	struct clk_init_data init;
+	struct clk_parent_data pdata = { .index = index };
 	struct clk_hw *hw;
 	int ret;
 
@@ -85,11 +87,17 @@  struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 	init.name = name;
 	init.ops = &clk_fixed_factor_ops;
 	init.flags = flags | CLK_IS_BASIC;
-	init.parent_names = &parent_name;
+	if (parent_name)
+		init.parent_names = &parent_name;
+	else
+		init.parent_data = &pdata;
 	init.num_parents = 1;
 
 	hw = &fix->hw;
-	ret = clk_hw_register(dev, hw);
+	if (dev)
+		ret = clk_hw_register(dev, hw);
+	else
+		ret = of_clk_hw_register(np, hw);
 	if (ret) {
 		kfree(fix);
 		hw = ERR_PTR(ret);
@@ -97,6 +105,14 @@  struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
 
 	return hw;
 }
+
+struct clk_hw *clk_hw_register_fixed_factor(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		unsigned int mult, unsigned int div)
+{
+	return __clk_hw_register_fixed_factor(dev, NULL, name, parent_name, -1, flags,
+					      mult, div);
+}
 EXPORT_SYMBOL_GPL(clk_hw_register_fixed_factor);
 
 struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
@@ -143,11 +159,10 @@  static const struct of_device_id set_rate_parent_matches[] = {
 	{ /* Sentinel */ },
 };
 
-static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
+static struct clk_hw *_of_fixed_factor_clk_setup(struct device_node *node)
 {
-	struct clk *clk;
+	struct clk_hw *hw;
 	const char *clk_name = node->name;
-	const char *parent_name;
 	unsigned long flags = 0;
 	u32 div, mult;
 	int ret;
@@ -165,30 +180,28 @@  static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
 	}
 
 	of_property_read_string(node, "clock-output-names", &clk_name);
-	parent_name = of_clk_get_parent_name(node, 0);
 
 	if (of_match_node(set_rate_parent_matches, node))
 		flags |= CLK_SET_RATE_PARENT;
 
-	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags,
-					mult, div);
-	if (IS_ERR(clk)) {
+	hw = __clk_hw_register_fixed_factor(NULL, node, clk_name, NULL, 0,
+					    flags, mult, div);
+	if (IS_ERR(hw)) {
 		/*
-		 * If parent clock is not registered, registration would fail.
 		 * Clear OF_POPULATED flag so that clock registration can be
 		 * attempted again from probe function.
 		 */
 		of_node_clear_flag(node, OF_POPULATED);
-		return clk;
+		return ERR_CAST(hw);
 	}
 
-	ret = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, hw);
 	if (ret) {
-		clk_unregister(clk);
+		clk_hw_unregister_fixed_factor(hw);
 		return ERR_PTR(ret);
 	}
 
-	return clk;
+	return hw;
 }
 
 /**
@@ -203,17 +216,17 @@  CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
 
 static int of_fixed_factor_clk_remove(struct platform_device *pdev)
 {
-	struct clk *clk = platform_get_drvdata(pdev);
+	struct clk_hw *clk = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(pdev->dev.of_node);
-	clk_unregister_fixed_factor(clk);
+	clk_hw_unregister_fixed_factor(clk);
 
 	return 0;
 }
 
 static int of_fixed_factor_clk_probe(struct platform_device *pdev)
 {
-	struct clk *clk;
+	struct clk_hw *clk;
 
 	/*
 	 * This function is not executed when of_fixed_factor_clk_setup