Re: [PATCH v2 3/3] powerpc: Convert fsl_rstcr_restart to a reset handler
On Tue, 9 Aug 2016 11:47:37 -0700 Andrey Smirnovwrote: > On Sun, Jul 31, 2016 at 9:03 PM, Nicholas Piggin wrote: > > On Thu, 28 Jul 2016 16:07:18 -0700 > > Andrey Smirnov wrote: > > > >> Convert fsl_rstcr_restart into a function to be registered with > >> register_reset_handler(). > >> > >> Signed-off-by: Andrey Smirnov > >> --- > >> > >> Changes since v1: > >> > >> - fsl_rstcr_restart is registered as a reset handler in > >> setup_rstcr, replacing per-board arch_initcall approach > > > > Bear in mind I don't know much about the embedded or platform code! > > > > The documentation for reset notifiers says that they are expected > > to be registered from drivers, not arch code. That seems to only be > > intended to mean that the standard ISA or platform reset would > > normally be handled directly by the arch, whereas if you have an > > arch specific driver for a reset hardware that just happens to live > > under arch/, then fsl_rstcr_restart / mpc85xx_cds_restart would be > > valid use of reset notifier. > > Yeah, IMHO there's quite a bit of code in sysdev/ which in ideal world > would go into drivers/ and I think fsl_rstcr_restart is among it > (similar example on MIPS is drivers/power/reset/brcmstb-reboot.c). > > > > > So this change seems reasonable to me. One small question: > > > > > >> +static int mpc85xx_cds_restart_register(void) > >> +{ > >> + static struct notifier_block restart_handler; > >> + > >> + restart_handler.notifier_call = mpc85xx_cds_restart; > >> + restart_handler.priority = 192; > > > > Should there be a header with #define's for these priorities? > > I don't have any strong preference either way, I do however think that > introducing such #define should go into a separate patch-set, since > you'd probably want to propagate that change across all of the users > of the API. You're probably right. I was thinking because powerpc has not used it before we could use local defines, but it really does need a global location. Thanks, Nick
Re: [PATCH v2 3/3] powerpc: Convert fsl_rstcr_restart to a reset handler
On Sun, Jul 31, 2016 at 9:03 PM, Nicholas Pigginwrote: > On Thu, 28 Jul 2016 16:07:18 -0700 > Andrey Smirnov wrote: > >> Convert fsl_rstcr_restart into a function to be registered with >> register_reset_handler(). >> >> Signed-off-by: Andrey Smirnov >> --- >> >> Changes since v1: >> >> - fsl_rstcr_restart is registered as a reset handler in >> setup_rstcr, replacing per-board arch_initcall approach > > Bear in mind I don't know much about the embedded or platform code! > > The documentation for reset notifiers says that they are expected > to be registered from drivers, not arch code. That seems to only be > intended to mean that the standard ISA or platform reset would > normally be handled directly by the arch, whereas if you have an > arch specific driver for a reset hardware that just happens to live > under arch/, then fsl_rstcr_restart / mpc85xx_cds_restart would be > valid use of reset notifier. Yeah, IMHO there's quite a bit of code in sysdev/ which in ideal world would go into drivers/ and I think fsl_rstcr_restart is among it (similar example on MIPS is drivers/power/reset/brcmstb-reboot.c). > > So this change seems reasonable to me. One small question: > > >> +static int mpc85xx_cds_restart_register(void) >> +{ >> + static struct notifier_block restart_handler; >> + >> + restart_handler.notifier_call = mpc85xx_cds_restart; >> + restart_handler.priority = 192; > > Should there be a header with #define's for these priorities? I don't have any strong preference either way, I do however think that introducing such #define should go into a separate patch-set, since you'd probably want to propagate that change across all of the users of the API. Thanks, Andrey
Re: [PATCH v2 3/3] powerpc: Convert fsl_rstcr_restart to a reset handler
On Thu, 28 Jul 2016 16:07:18 -0700 Andrey Smirnovwrote: > Convert fsl_rstcr_restart into a function to be registered with > register_reset_handler(). > > Signed-off-by: Andrey Smirnov > --- > > Changes since v1: > > - fsl_rstcr_restart is registered as a reset handler in > setup_rstcr, replacing per-board arch_initcall approach Bear in mind I don't know much about the embedded or platform code! The documentation for reset notifiers says that they are expected to be registered from drivers, not arch code. That seems to only be intended to mean that the standard ISA or platform reset would normally be handled directly by the arch, whereas if you have an arch specific driver for a reset hardware that just happens to live under arch/, then fsl_rstcr_restart / mpc85xx_cds_restart would be valid use of reset notifier. So this change seems reasonable to me. One small question: > +static int mpc85xx_cds_restart_register(void) > +{ > + static struct notifier_block restart_handler; > + > + restart_handler.notifier_call = mpc85xx_cds_restart; > + restart_handler.priority = 192; Should there be a header with #define's for these priorities? Thanks, Nick ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 3/3] powerpc: Convert fsl_rstcr_restart to a reset handler
Convert fsl_rstcr_restart into a function to be registered with register_reset_handler(). Signed-off-by: Andrey Smirnov--- Changes since v1: - fsl_rstcr_restart is registered as a reset handler in setup_rstcr, replacing per-board arch_initcall approach arch/powerpc/platforms/85xx/bsc913x_qds.c | 1 - arch/powerpc/platforms/85xx/bsc913x_rdb.c | 1 - arch/powerpc/platforms/85xx/c293pcie.c| 1 - arch/powerpc/platforms/85xx/corenet_generic.c | 1 - arch/powerpc/platforms/85xx/ge_imp3a.c| 1 - arch/powerpc/platforms/85xx/mpc8536_ds.c | 1 - arch/powerpc/platforms/85xx/mpc85xx_ads.c | 1 - arch/powerpc/platforms/85xx/mpc85xx_cds.c | 25 ++-- arch/powerpc/platforms/85xx/mpc85xx_ds.c | 3 --- arch/powerpc/platforms/85xx/mpc85xx_mds.c | 3 --- arch/powerpc/platforms/85xx/mpc85xx_rdb.c | 10 arch/powerpc/platforms/85xx/mvme2500.c| 1 - arch/powerpc/platforms/85xx/p1010rdb.c| 1 - arch/powerpc/platforms/85xx/p1022_ds.c| 1 - arch/powerpc/platforms/85xx/p1022_rdk.c | 1 - arch/powerpc/platforms/85xx/p1023_rdb.c | 1 - arch/powerpc/platforms/85xx/ppa8548.c | 1 - arch/powerpc/platforms/85xx/qemu_e500.c | 1 - arch/powerpc/platforms/85xx/sbc8548.c | 1 - arch/powerpc/platforms/85xx/socrates.c| 1 - arch/powerpc/platforms/85xx/stx_gp3.c | 1 - arch/powerpc/platforms/85xx/tqm85xx.c | 1 - arch/powerpc/platforms/85xx/twr_p102x.c | 1 - arch/powerpc/platforms/85xx/xes_mpc85xx.c | 3 --- arch/powerpc/platforms/86xx/gef_ppc9a.c | 1 - arch/powerpc/platforms/86xx/gef_sbc310.c | 1 - arch/powerpc/platforms/86xx/gef_sbc610.c | 1 - arch/powerpc/platforms/86xx/mpc8610_hpcd.c| 1 - arch/powerpc/platforms/86xx/mpc86xx_hpcn.c| 1 - arch/powerpc/platforms/86xx/sbc8641d.c| 1 - arch/powerpc/sysdev/fsl_soc.c | 33 --- arch/powerpc/sysdev/fsl_soc.h | 2 -- 32 files changed, 38 insertions(+), 66 deletions(-) diff --git a/arch/powerpc/platforms/85xx/bsc913x_qds.c b/arch/powerpc/platforms/85xx/bsc913x_qds.c index 07dd6ae..d2f4556 100644 --- a/arch/powerpc/platforms/85xx/bsc913x_qds.c +++ b/arch/powerpc/platforms/85xx/bsc913x_qds.c @@ -72,7 +72,6 @@ define_machine(bsc9132_qds) { .pcibios_fixup_bus = fsl_pcibios_fixup_bus, #endif .get_irq= mpic_get_irq, - .restart= fsl_rstcr_restart, .calibrate_decr = generic_calibrate_decr, .progress = udbg_progress, }; diff --git a/arch/powerpc/platforms/85xx/bsc913x_rdb.c b/arch/powerpc/platforms/85xx/bsc913x_rdb.c index e48f671..0ffdb4a 100644 --- a/arch/powerpc/platforms/85xx/bsc913x_rdb.c +++ b/arch/powerpc/platforms/85xx/bsc913x_rdb.c @@ -59,7 +59,6 @@ define_machine(bsc9131_rdb) { .setup_arch = bsc913x_rdb_setup_arch, .init_IRQ = bsc913x_rdb_pic_init, .get_irq= mpic_get_irq, - .restart= fsl_rstcr_restart, .calibrate_decr = generic_calibrate_decr, .progress = udbg_progress, }; diff --git a/arch/powerpc/platforms/85xx/c293pcie.c b/arch/powerpc/platforms/85xx/c293pcie.c index 3b9e3f0..4df1b40 100644 --- a/arch/powerpc/platforms/85xx/c293pcie.c +++ b/arch/powerpc/platforms/85xx/c293pcie.c @@ -65,7 +65,6 @@ define_machine(c293_pcie) { .setup_arch = c293_pcie_setup_arch, .init_IRQ = c293_pcie_pic_init, .get_irq= mpic_get_irq, - .restart= fsl_rstcr_restart, .calibrate_decr = generic_calibrate_decr, .progress = udbg_progress, }; diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c b/arch/powerpc/platforms/85xx/corenet_generic.c index 3a6a84f..1179115 100644 --- a/arch/powerpc/platforms/85xx/corenet_generic.c +++ b/arch/powerpc/platforms/85xx/corenet_generic.c @@ -225,7 +225,6 @@ define_machine(corenet_generic) { #else .get_irq= mpic_get_coreint_irq, #endif - .restart= fsl_rstcr_restart, .calibrate_decr = generic_calibrate_decr, .progress = udbg_progress, #ifdef CONFIG_PPC64 diff --git a/arch/powerpc/platforms/85xx/ge_imp3a.c b/arch/powerpc/platforms/85xx/ge_imp3a.c index 14af36a..f29c6f0 100644 --- a/arch/powerpc/platforms/85xx/ge_imp3a.c +++ b/arch/powerpc/platforms/85xx/ge_imp3a.c @@ -215,7 +215,6 @@ define_machine(ge_imp3a) { .pcibios_fixup_phb = fsl_pcibios_fixup_phb, #endif .get_irq= mpic_get_irq, - .restart= fsl_rstcr_restart, .calibrate_decr = generic_calibrate_decr, .progress = udbg_progress, }; diff --git