[PATCH 0/2] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[PATCH 0/2] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[Kevin Hao]

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

[PATCH 1/2] powerpc: move ppc_init() to common place

[Kevin Hao]

So they can be used by ppc64 board. Also remove the unneeded {} to
make checkpatch happy.

Signed-off-by: Kevin Hao <haokexin@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

Re: [PATCH 1/2] powerpc: move ppc_init() to common place

[Scott Wood]

On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> So they can be used by ppc64 board. Also remove the unneeded {} to
> make checkpatch happy.
> 
> Signed-off-by: Kevin Hao <haokexin@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

Re: [PATCH 1/2] powerpc: move ppc_init() to common place

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Thu, Oct 16, 2014 at 11:54:46PM +0200, Scott Wood wrote:
> On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> > So they can be used by ppc64 board. Also remove the unneeded {} to
> > make checkpatch happy.
> > 
> > Signed-off-by: Kevin Hao <haokexin@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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[PATCH 2/2] clk: ppc-corenet: don't use platform_driver to init the clock device

[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@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

Re: [PATCH 2/2] clk: ppc-corenet: don't use platform_driver to init the clock device

[Scott Wood]

On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> 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@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

Re: [PATCH 2/2] clk: ppc-corenet: don't use platform_driver to init the clock device

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Thu, Oct 16, 2014 at 11:55:23PM +0200, Scott Wood wrote:
> On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> > 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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] clk: ppc-corenet: don't use platform_driver to init the clock device

[Scott Wood]

On Fri, 2014-10-17 at 06:55 +0800, Kevin Hao wrote:
> On Thu, Oct 16, 2014 at 11:55:23PM +0200, Scott Wood wrote:
> > On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> > > 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

Re: [PATCH 2/2] clk: ppc-corenet: don't use platform_driver to init the clock device

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

On Fri, Oct 17, 2014 at 07:58:55AM +0200, Scott Wood wrote:
> On Fri, 2014-10-17 at 06:55 +0800, Kevin Hao wrote:
> > 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()?

I have took a look at the mpc5121_clk_init() and it seems that the
of_clk_init() is only used to scan the 'fixed-clock' in the DT. So I think
it should be safe to invoke it a bit earlier in a common place. I will create
v2 to use this method.

Gerhard, please correct me if I am wrong.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/2] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[Scott Wood]

On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> 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

Re: [PATCH 0/2] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

On Thu, Oct 16, 2014 at 11:49:22PM +0200, Scott Wood wrote:
> On Thu, 2014-10-16 at 19:18 +0800, Kevin Hao wrote:
> > 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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]