Discussion:
[PATCH 0/2] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled
Kevin Hao
2014-10-16 11:18:39 UTC
Permalink
Hi,

This tries to fix a kernel panic introduced by commit da788acb2838
("clk: ppc-corenet: Fix Section mismatch warning").

Kevin Hao (2):
powerpc: move ppc_init() to common place
clk: ppc-corenet: don't use platform_driver to init the clock device

arch/powerpc/include/asm/machdep.h | 2 +-
arch/powerpc/kernel/setup-common.c | 15 ++++++++++
arch/powerpc/kernel/setup_32.c | 15 ----------
arch/powerpc/platforms/85xx/corenet_generic.c | 7 +++++
drivers/clk/clk-ppc-corenet.c | 43 ++++-----------------------
5 files changed, 29 insertions(+), 53 deletions(-)
--
1.9.3
Kevin Hao
2014-10-16 11:18:40 UTC
Permalink
So they can be used by ppc64 board. Also remove the unneeded {} to
make checkpatch happy.

Signed-off-by: Kevin Hao <haokexin at gmail.com>
---
arch/powerpc/include/asm/machdep.h | 2 +-
arch/powerpc/kernel/setup-common.c | 15 +++++++++++++++
arch/powerpc/kernel/setup_32.c | 15 ---------------
3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 307347f8ddbd..4b913d53333b 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -213,11 +213,11 @@ struct machdep_calls {
int (*set_dawr)(unsigned long dawr,
unsigned long dawrx);

-#ifdef CONFIG_PPC32 /* XXX for now */
/* A general init function, called by ppc_init in init/main.c.
May be NULL. */
void (*init)(void);

+#ifdef CONFIG_PPC32 /* XXX for now */
void (*kgdb_map_scc)(void);

/*
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 1362cd62b3fa..f76d4a1dbe73 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -750,3 +750,18 @@ void arch_setup_pdev_archdata(struct platform_device *pdev)
pdev->dev.dma_mask = &pdev->archdata.dma_mask;
set_dma_ops(&pdev->dev, &dma_direct_ops);
}
+
+int __init ppc_init(void)
+{
+ /* clear the progress line */
+ if (ppc_md.progress)
+ ppc_md.progress(" ", 0xffff);
+
+ /* call platform init */
+ if (ppc_md.init != NULL)
+ ppc_md.init();
+
+ return 0;
+}
+
+arch_initcall(ppc_init);
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 07831ed0d9ef..59d8c6e15782 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -208,21 +208,6 @@ EXPORT_SYMBOL(nvram_sync);

#endif /* CONFIG_NVRAM */

-int __init ppc_init(void)
-{
- /* clear the progress line */
- if (ppc_md.progress)
- ppc_md.progress(" ", 0xffff);
-
- /* call platform init */
- if (ppc_md.init != NULL) {
- ppc_md.init();
- }
- return 0;
-}
-
-arch_initcall(ppc_init);
-
static void __init irqstack_early_init(void)
{
unsigned int i;
--
1.9.3
Scott Wood
2014-10-16 21:54:46 UTC
Permalink
Post by Kevin Hao
So they can be used by ppc64 board. Also remove the unneeded {} to
make checkpatch happy.
Signed-off-by: Kevin Hao <haokexin at gmail.com>
---
arch/powerpc/include/asm/machdep.h | 2 +-
arch/powerpc/kernel/setup-common.c | 15 +++++++++++++++
arch/powerpc/kernel/setup_32.c | 15 ---------------
3 files changed, 16 insertions(+), 16 deletions(-)
This is unnecessary -- why not just use machine_arch_initcall?

-Scott
Kevin Hao
2014-10-16 22:34:58 UTC
Permalink
Post by Scott Wood
Post by Kevin Hao
So they can be used by ppc64 board. Also remove the unneeded {} to
make checkpatch happy.
Signed-off-by: Kevin Hao <haokexin at gmail.com>
---
arch/powerpc/include/asm/machdep.h | 2 +-
arch/powerpc/kernel/setup-common.c | 15 +++++++++++++++
arch/powerpc/kernel/setup_32.c | 15 ---------------
3 files changed, 16 insertions(+), 16 deletions(-)
This is unnecessary -- why not just use machine_arch_initcall?
OK, I will drop this patch.

Thanks,
Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20141017/cca5aaf9/attachment.sig>
Kevin Hao
2014-10-16 11:18:41 UTC
Permalink
In commit da788acb2838 ("clk: ppc-corenet: Fix Section mismatch
warning"), we put the ppc_corenet_clk_driver struct to init section
in order to fix section mismatch warning. This is definitely wrong
because the kernel would free the memories occupied by
ppc_corenet_clk_driver after boot while this driver is still registered
in the driver core. The kernel would panic when accessing this driver
struct. So choose to use CLK_OF_DECLARE to scan and init the clock devices.

Signed-off-by: Kevin Hao <haokexin at gmail.com>
---
arch/powerpc/platforms/85xx/corenet_generic.c | 7 +++++
drivers/clk/clk-ppc-corenet.c | 43 ++++-----------------------
2 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index e56b89a792ed..7677cfecb787 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c
@@ -16,6 +16,7 @@
#include <linux/kdev_t.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
+#include <linux/clk-provider.h>

#include <asm/time.h>
#include <asm/machdep.h>
@@ -188,11 +189,17 @@ static int __init corenet_generic_probe(void)
return 0;
}

+static void __init corenet_gen_init(void)
+{
+ of_clk_init(NULL);
+}
+
define_machine(corenet_generic) {
.name = "CoreNet Generic",
.probe = corenet_generic_probe,
.setup_arch = corenet_gen_setup_arch,
.init_IRQ = corenet_gen_pic_init,
+ .init = corenet_gen_init,
#ifdef CONFIG_PCI
.pcibios_fixup_bus = fsl_pcibios_fixup_bus,
.pcibios_fixup_phb = fsl_pcibios_fixup_phb,
diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c
index 8e58edfeeb37..bf0fe565ce4e 100644
--- a/drivers/clk/clk-ppc-corenet.c
+++ b/drivers/clk/clk-ppc-corenet.c
@@ -268,40 +268,9 @@ static void __init sysclk_init(struct device_node *node)
of_clk_add_provider(np, of_clk_src_simple_get, clk);
}

-static const struct of_device_id clk_match[] __initconst = {
- { .compatible = "fsl,qoriq-sysclk-1.0", .data = sysclk_init, },
- { .compatible = "fsl,qoriq-sysclk-2.0", .data = sysclk_init, },
- { .compatible = "fsl,qoriq-core-pll-1.0", .data = core_pll_init, },
- { .compatible = "fsl,qoriq-core-pll-2.0", .data = core_pll_init, },
- { .compatible = "fsl,qoriq-core-mux-1.0", .data = core_mux_init, },
- { .compatible = "fsl,qoriq-core-mux-2.0", .data = core_mux_init, },
- {}
-};
-
-static int __init ppc_corenet_clk_probe(struct platform_device *pdev)
-{
- of_clk_init(clk_match);
-
- return 0;
-}
-
-static const struct of_device_id ppc_clk_ids[] __initconst = {
- { .compatible = "fsl,qoriq-clockgen-1.0", },
- { .compatible = "fsl,qoriq-clockgen-2.0", },
- {}
-};
-
-static struct platform_driver ppc_corenet_clk_driver __initdata = {
- .driver = {
- .name = "ppc_corenet_clock",
- .owner = THIS_MODULE,
- .of_match_table = ppc_clk_ids,
- },
- .probe = ppc_corenet_clk_probe,
-};
-
-static int __init ppc_corenet_clk_init(void)
-{
- return platform_driver_register(&ppc_corenet_clk_driver);
-}
-subsys_initcall(ppc_corenet_clk_init);
+CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init);
+CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init);
+CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", core_pll_init);
+CLK_OF_DECLARE(qoriq_core_pll_2, "fsl,qoriq-core-pll-2.0", core_pll_init);
+CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init);
+CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init);
--
1.9.3
Scott Wood
2014-10-16 21:55:23 UTC
Permalink
Post by Kevin Hao
In commit da788acb2838 ("clk: ppc-corenet: Fix Section mismatch
warning"), we put the ppc_corenet_clk_driver struct to init section
in order to fix section mismatch warning. This is definitely wrong
because the kernel would free the memories occupied by
ppc_corenet_clk_driver after boot while this driver is still registered
in the driver core. The kernel would panic when accessing this driver
struct. So choose to use CLK_OF_DECLARE to scan and init the clock devices.
Signed-off-by: Kevin Hao <haokexin at gmail.com>
---
arch/powerpc/platforms/85xx/corenet_generic.c | 7 +++++
drivers/clk/clk-ppc-corenet.c | 43 ++++-----------------------
2 files changed, 13 insertions(+), 37 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index e56b89a792ed..7677cfecb787 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c
@@ -16,6 +16,7 @@
#include <linux/kdev_t.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
+#include <linux/clk-provider.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -188,11 +189,17 @@ static int __init corenet_generic_probe(void)
return 0;
}
+static void __init corenet_gen_init(void)
+{
+ of_clk_init(NULL);
+}
Why is this board-specific?

-Scott
Kevin Hao
2014-10-16 22:55:52 UTC
Permalink
Post by Scott Wood
Post by Kevin Hao
diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index e56b89a792ed..7677cfecb787 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c
@@ -16,6 +16,7 @@
#include <linux/kdev_t.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
+#include <linux/clk-provider.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -188,11 +189,17 @@ static int __init corenet_generic_probe(void)
return 0;
}
+static void __init corenet_gen_init(void)
+{
+ of_clk_init(NULL);
+}
Why is this board-specific?
I have thought about to put it in a more common place such as time_init(),
but this will be in conflict with mpc512x board. How about add an
arch_initcall(mpc85xx_clk_init) in arch/powerpc/platforms/85xx/common.c?

Thanks,
Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20141017/3095b9d1/attachment.sig>
Scott Wood
2014-10-17 05:58:55 UTC
Permalink
Post by Kevin Hao
Post by Scott Wood
Post by Kevin Hao
diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c
index e56b89a792ed..7677cfecb787 100644
--- a/arch/powerpc/platforms/85xx/corenet_generic.c
+++ b/arch/powerpc/platforms/85xx/corenet_generic.c
@@ -16,6 +16,7 @@
#include <linux/kdev_t.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
+#include <linux/clk-provider.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -188,11 +189,17 @@ static int __init corenet_generic_probe(void)
return 0;
}
+static void __init corenet_gen_init(void)
+{
+ of_clk_init(NULL);
+}
Why is this board-specific?
I have thought about to put it in a more common place such as time_init(),
but this will be in conflict with mpc512x board. How about add an
arch_initcall(mpc85xx_clk_init) in arch/powerpc/platforms/85xx/common.c?
Gerhard, does 512x really require of_clk_init() to be called at that
specific time, or can it be replaced by a common of_clk_init()?

-Scott

Scott Wood
2014-10-16 21:49:22 UTC
Permalink
Post by Kevin Hao
Hi,
This tries to fix a kernel panic introduced by commit da788acb2838
("clk: ppc-corenet: Fix Section mismatch warning").
That patch is just wrong and should be reverted, separately from any new
attempt to fix the section mismatch warning.

-Scott
Kevin Hao
2014-10-16 22:31:24 UTC
Permalink
Post by Scott Wood
Post by Kevin Hao
Hi,
This tries to fix a kernel panic introduced by commit da788acb2838
("clk: ppc-corenet: Fix Section mismatch warning").
That patch is just wrong and should be reverted, separately from any new
attempt to fix the section mismatch warning.
OK, I will make a patch to revert that first.

Thanks,
Kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20141017/c7a21c50/attachment.sig>
Loading...