| Message ID | 1462079316-27771-6-git-send-email-ysato@users.sourceforge.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: > Changes bellow > - FDT setup timing fix. > - chosen/bootargs support. > - zImage support. > - DT binding helper macro. > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > --- > arch/sh/boards/of-generic.c | 23 +++++++++++----------- > arch/sh/boot/compressed/head_32.S | 5 +++-- > arch/sh/boot/dts/include/dt-bindings | 1 + > arch/sh/kernel/setup.c | 19 ++++++++++++++++++ > include/dt-bindings/interrupt-controller/sh_intc.h | 2 ++ > 5 files changed, 36 insertions(+), 14 deletions(-) > create mode 120000 arch/sh/boot/dts/include/dt-bindings > create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h > > diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c > index bf3a166..9570873 100644 > --- a/arch/sh/boards/of-generic.c > +++ b/arch/sh/boards/of-generic.c > @@ -112,29 +112,25 @@ static int noopi(void) > return 0; > } > > -static void __init sh_of_mem_reserve(void) > +static void __init sh_of_mem_init(void) > { > early_init_fdt_reserve_self(); > early_init_fdt_scan_reserved_mem(); > } > > -static void __init sh_of_time_init(void) > -{ > - pr_info("SH generic board support: scanning for clocksource devices\n"); > - clocksource_probe(); > -} Why did you remove this? Without it you won't get clock event/clocksource devices from the device tree so the only way to have a working timer interrupt is if the driver is hard-coded somewhere. > static void __init sh_of_setup(char **cmdline_p) > { > - unflatten_device_tree(); > - > - board_time_init = sh_of_time_init; > + struct device_node *cpu; > + int freq; > > sh_mv.mv_name = of_flat_dt_get_machine_name(); > if (!sh_mv.mv_name) > sh_mv.mv_name = "Unknown SH model"; > > sh_of_smp_probe(); > + cpu = of_find_node_by_name(NULL, "cpu"); > + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) > + preset_lpj = freq / 500; > } I setup the DT-based pseudo-board to use the generic calibrate-delay rather than hard-coding lpj. Ideally we could just get rid of bogomips completely but there are probably still some things using it. Is there a reason you prefer making up a value for lpj based on the cpu clock rate? > static int sh_of_irq_demux(int irq) > @@ -167,8 +163,7 @@ static struct sh_machine_vector __initmv sh_of_generic_mv = { > .mv_init_irq = sh_of_init_irq, > .mv_clk_init = sh_of_clk_init, > .mv_mode_pins = noopi, > - .mv_mem_init = noop, > - .mv_mem_reserve = sh_of_mem_reserve, > + .mv_mem_init = sh_of_mem_init, Is there a reason for this renaming? The function seems to be dealing purely with reserving memory ranges. > struct sh_clk_ops; > @@ -194,3 +189,7 @@ static int __init sh_of_device_init(void) > return 0; > } > arch_initcall_sync(sh_of_device_init); > + > +void intc_finalize(void) > +{ > +} > diff --git a/arch/sh/boot/compressed/head_32.S b/arch/sh/boot/compressed/head_32.S > index 3e15032..cd34377 100644 > --- a/arch/sh/boot/compressed/head_32.S > +++ b/arch/sh/boot/compressed/head_32.S > @@ -14,7 +14,8 @@ startup: > /* Load initial status register */ > mov.l init_sr, r1 > ldc r1, sr > - > + /* Save FDT address */ > + mov r4, r13 > /* Move myself to proper location if necessary */ > mova 1f, r0 > mov.l 1f, r2 > @@ -83,7 +84,7 @@ l1: > /* Jump to the start of the decompressed kernel */ > mov.l kernel_start_addr, r0 > jmp @r0 > - nop > + mov r13,r4 > > .align 2 > bss_start_addr: This should probably be its own patch, adding DT support for compressed kernel images. It's independent from everything else in this patch. > diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings > new file mode 120000 > index 0000000..08c00e4 > --- /dev/null > +++ b/arch/sh/boot/dts/include/dt-bindings > @@ -0,0 +1 @@ > +../../../../../include/dt-bindings > \ No newline at end of file > diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c > index 5d34605..f6bb105 100644 > --- a/arch/sh/kernel/setup.c > +++ b/arch/sh/kernel/setup.c > @@ -177,7 +177,12 @@ disable: > #ifndef CONFIG_GENERIC_CALIBRATE_DELAY > void calibrate_delay(void) > { > +#ifndef CONFIG_OF > struct clk *clk = clk_get(NULL, "cpu_clk"); > +#else > + struct device_node *cpu = of_find_node_by_name(NULL, "cpu"); > + struct clk *clk = of_clk_get_by_name(cpu, NULL); > +#endif > > if (IS_ERR(clk)) > panic("Need a sane CPU clock definition!"); > @@ -251,7 +256,11 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > /* Avoid calling an __init function on secondary cpus. */ > if (done) return; > > +#ifdef CONFIG_USE_BUILTIN_DTB > + dt_virt = __dtb_start; > +#else > dt_virt = phys_to_virt(dt_phys); > +#endif This is also part of the bultin-dtb patch, which seems to have been spread out across several of your patches. > if (!dt_virt || !early_init_dt_scan(dt_virt)) { > pr_crit("Error: invalid device tree blob" > @@ -267,8 +276,13 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > > void __init setup_arch(char **cmdline_p) > { > +#ifdef CONFIG_OF > + unflatten_device_tree(); > +#endif > enable_mmu(); Was this moved to setup_arch to have it before enable_mmu? I think that makes sense. > +#ifndef CONFIG_OF > ROOT_DEV = old_decode_dev(ORIG_ROOT_DEV); > > printk(KERN_NOTICE "Boot params:\n" > @@ -290,6 +304,7 @@ void __init setup_arch(char **cmdline_p) > > if (!MOUNT_ROOT_RDONLY) > root_mountflags &= ~MS_RDONLY; > +#endif Do these boot params only make sense for non-DT setups? > +#if !defined(CONFIG_OF) || defined(USE_BUILTIN_DTB) > /* Save unparsed command line copy for /proc/cmdline */ > memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE); > *cmdline_p = command_line; > +#else > + *cmdline_p = boot_command_line; > +#endif I think this is wrong -- it causes the builtin command line and bootloader-provided command line to be ignored on DT kernels. Do you just want to deprecate builtin and bootloader-provided command lines? Or is it just a side effect of adding support for chosen/bootarg? > parse_early_param(); > > diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h > new file mode 100644 > index 0000000..8c9dcdc > --- /dev/null > +++ b/include/dt-bindings/interrupt-controller/sh_intc.h > @@ -0,0 +1,2 @@ > +#define evt2irq(evt) (((evt) >> 5) - 16) > +#define irq2evt(irq) (((irq) + 16) << 5) This seems unrelated to other things in this patch. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 4, 2016 at 5:10 AM, Rich Felker <dalias@libc.org> wrote: >On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: >> static void __init sh_of_setup(char **cmdline_p) >> { >> - unflatten_device_tree(); >> - >> - board_time_init = sh_of_time_init; >> + struct device_node *cpu; >> + int freq; You better make freq unsigned. >> sh_mv.mv_name = of_flat_dt_get_machine_name(); >> if (!sh_mv.mv_name) >> sh_mv.mv_name = "Unknown SH model"; >> >> sh_of_smp_probe(); >> + cpu = of_find_node_by_name(NULL, "cpu"); >> + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) >> + preset_lpj = freq / 500; >> } > > I setup the DT-based pseudo-board to use the generic calibrate-delay > rather than hard-coding lpj. Ideally we could just get rid of bogomips > completely but there are probably still some things using it. Is there > a reason you prefer making up a value for lpj based on the cpu clock > rate? Calibrating the delay loop takes some time. However, you shouldn't hardcode 500, but take HZ into account. I assume you used the default HZ=250, so preset_lpj = freq / HZ / 2; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 04 May 2016 12:10:05 +0900, Rich Felker wrote: > > On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: > > Changes bellow > > - FDT setup timing fix. > > - chosen/bootargs support. > > - zImage support. > > - DT binding helper macro. > > > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > > --- > > arch/sh/boards/of-generic.c | 23 +++++++++++----------- > > arch/sh/boot/compressed/head_32.S | 5 +++-- > > arch/sh/boot/dts/include/dt-bindings | 1 + > > arch/sh/kernel/setup.c | 19 ++++++++++++++++++ > > include/dt-bindings/interrupt-controller/sh_intc.h | 2 ++ > > 5 files changed, 36 insertions(+), 14 deletions(-) > > create mode 120000 arch/sh/boot/dts/include/dt-bindings > > create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h > > > > diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c > > index bf3a166..9570873 100644 > > --- a/arch/sh/boards/of-generic.c > > +++ b/arch/sh/boards/of-generic.c > > @@ -112,29 +112,25 @@ static int noopi(void) > > return 0; > > } > > > > -static void __init sh_of_mem_reserve(void) > > +static void __init sh_of_mem_init(void) > > { > > early_init_fdt_reserve_self(); > > early_init_fdt_scan_reserved_mem(); > > } > > > > -static void __init sh_of_time_init(void) > > -{ > > - pr_info("SH generic board support: scanning for clocksource devices\n"); > > - clocksource_probe(); > > -} > > Why did you remove this? Without it you won't get clock > event/clocksource devices from the device tree so the only way to have > a working timer interrupt is if the driver is hard-coded somewhere. It not needed on Common Clock Framework. tmu define in dts. > > static void __init sh_of_setup(char **cmdline_p) > > { > > - unflatten_device_tree(); > > - > > - board_time_init = sh_of_time_init; > > + struct device_node *cpu; > > + int freq; > > > > sh_mv.mv_name = of_flat_dt_get_machine_name(); > > if (!sh_mv.mv_name) > > sh_mv.mv_name = "Unknown SH model"; > > > > sh_of_smp_probe(); > > + cpu = of_find_node_by_name(NULL, "cpu"); > > + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) > > + preset_lpj = freq / 500; > > } > > I setup the DT-based pseudo-board to use the generic calibrate-delay > rather than hard-coding lpj. Ideally we could just get rid of bogomips > completely but there are probably still some things using it. Is there > a reason you prefer making up a value for lpj based on the cpu clock > rate? clockevent initalize after calibrate delay. So don't work interrupt based calibrate. > > static int sh_of_irq_demux(int irq) > > @@ -167,8 +163,7 @@ static struct sh_machine_vector __initmv sh_of_generic_mv = { > > .mv_init_irq = sh_of_init_irq, > > .mv_clk_init = sh_of_clk_init, > > .mv_mode_pins = noopi, > > - .mv_mem_init = noop, > > - .mv_mem_reserve = sh_of_mem_reserve, > > + .mv_mem_init = sh_of_mem_init, > > Is there a reason for this renaming? The function seems to be dealing > purely with reserving memory ranges. mv_mem_reserve too late call in MMU system. > > > struct sh_clk_ops; > > @@ -194,3 +189,7 @@ static int __init sh_of_device_init(void) > > return 0; > > } > > arch_initcall_sync(sh_of_device_init); > > + > > +void intc_finalize(void) > > +{ > > +} > > diff --git a/arch/sh/boot/compressed/head_32.S b/arch/sh/boot/compressed/head_32.S > > index 3e15032..cd34377 100644 > > --- a/arch/sh/boot/compressed/head_32.S > > +++ b/arch/sh/boot/compressed/head_32.S > > @@ -14,7 +14,8 @@ startup: > > /* Load initial status register */ > > mov.l init_sr, r1 > > ldc r1, sr > > - > > + /* Save FDT address */ > > + mov r4, r13 > > /* Move myself to proper location if necessary */ > > mova 1f, r0 > > mov.l 1f, r2 > > @@ -83,7 +84,7 @@ l1: > > /* Jump to the start of the decompressed kernel */ > > mov.l kernel_start_addr, r0 > > jmp @r0 > > - nop > > + mov r13,r4 > > > > .align 2 > > bss_start_addr: > > This should probably be its own patch, adding DT support for > compressed kernel images. It's independent from everything else in > this patch. OK. separated. > > diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings > > new file mode 120000 > > index 0000000..08c00e4 > > --- /dev/null > > +++ b/arch/sh/boot/dts/include/dt-bindings > > @@ -0,0 +1 @@ > > +../../../../../include/dt-bindings > > \ No newline at end of file > > diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c > > index 5d34605..f6bb105 100644 > > --- a/arch/sh/kernel/setup.c > > +++ b/arch/sh/kernel/setup.c > > @@ -177,7 +177,12 @@ disable: > > #ifndef CONFIG_GENERIC_CALIBRATE_DELAY > > void calibrate_delay(void) > > { > > +#ifndef CONFIG_OF > > struct clk *clk = clk_get(NULL, "cpu_clk"); > > +#else > > + struct device_node *cpu = of_find_node_by_name(NULL, "cpu"); > > + struct clk *clk = of_clk_get_by_name(cpu, NULL); > > +#endif > > > > if (IS_ERR(clk)) > > panic("Need a sane CPU clock definition!"); > > @@ -251,7 +256,11 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > > /* Avoid calling an __init function on secondary cpus. */ > > if (done) return; > > > > +#ifdef CONFIG_USE_BUILTIN_DTB > > + dt_virt = __dtb_start; > > +#else > > dt_virt = phys_to_virt(dt_phys); > > +#endif > > This is also part of the bultin-dtb patch, which seems to have been > spread out across several of your patches. Sorry. It my mistake. > > if (!dt_virt || !early_init_dt_scan(dt_virt)) { > > pr_crit("Error: invalid device tree blob" > > @@ -267,8 +276,13 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > > > > void __init setup_arch(char **cmdline_p) > > { > > +#ifdef CONFIG_OF > > + unflatten_device_tree(); > > +#endif > > enable_mmu(); > > Was this moved to setup_arch to have it before enable_mmu? I think > that makes sense. early_init_dt_alloc_memory_arch used physical address. It override on sh-specific, can after enable_mmu. But I don't feel an advantage. > > +#ifndef CONFIG_OF > > ROOT_DEV = old_decode_dev(ORIG_ROOT_DEV); > > > > printk(KERN_NOTICE "Boot params:\n" > > @@ -290,6 +304,7 @@ void __init setup_arch(char **cmdline_p) > > > > if (!MOUNT_ROOT_RDONLY) > > root_mountflags &= ~MS_RDONLY; > > +#endif > > Do these boot params only make sense for non-DT setups? > > > +#if !defined(CONFIG_OF) || defined(USE_BUILTIN_DTB) > > /* Save unparsed command line copy for /proc/cmdline */ > > memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE); > > *cmdline_p = command_line; > > +#else > > + *cmdline_p = boot_command_line; > > +#endif > > I think this is wrong -- it causes the builtin command line and > bootloader-provided command line to be ignored on DT kernels. Do you > just want to deprecate builtin and bootloader-provided command lines? > Or is it just a side effect of adding support for chosen/bootarg? Yes. I think zero-page passing only non-DT. DT using chosen/bootargs. > > parse_early_param(); > > > > diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h > > new file mode 100644 > > index 0000000..8c9dcdc > > --- /dev/null > > +++ b/include/dt-bindings/interrupt-controller/sh_intc.h > > @@ -0,0 +1,2 @@ > > +#define evt2irq(evt) (((evt) >> 5) - 16) > > +#define irq2evt(irq) (((irq) + 16) << 5) > > This seems unrelated to other things in this patch. It using DT define. > Rich
On Wed, 04 May 2016 15:41:18 +0900, Geert Uytterhoeven wrote: > > On Wed, May 4, 2016 at 5:10 AM, Rich Felker <dalias@libc.org> wrote: > >On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: > >> static void __init sh_of_setup(char **cmdline_p) > >> { > >> - unflatten_device_tree(); > >> - > >> - board_time_init = sh_of_time_init; > >> + struct device_node *cpu; > >> + int freq; > > You better make freq unsigned. OK. > >> sh_mv.mv_name = of_flat_dt_get_machine_name(); > >> if (!sh_mv.mv_name) > >> sh_mv.mv_name = "Unknown SH model"; > >> > >> sh_of_smp_probe(); > >> + cpu = of_find_node_by_name(NULL, "cpu"); > >> + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) > >> + preset_lpj = freq / 500; > >> } > > > > I setup the DT-based pseudo-board to use the generic calibrate-delay > > rather than hard-coding lpj. Ideally we could just get rid of bogomips > > completely but there are probably still some things using it. Is there > > a reason you prefer making up a value for lpj based on the cpu clock > > rate? > > Calibrating the delay loop takes some time. > However, you shouldn't hardcode 500, but take HZ into account. > I assume you used the default HZ=250, so That's right. I'll fix it. > preset_lpj = freq / HZ / 2; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Tue, May 10, 2016 at 05:25:36PM +0900, Yoshinori Sato wrote: > On Wed, 04 May 2016 12:10:05 +0900, > Rich Felker wrote: > > > > On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: > > > Changes bellow > > > - FDT setup timing fix. > > > - chosen/bootargs support. > > > - zImage support. > > > - DT binding helper macro. > > > > > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > > > --- > > > arch/sh/boards/of-generic.c | 23 +++++++++++----------- > > > arch/sh/boot/compressed/head_32.S | 5 +++-- > > > arch/sh/boot/dts/include/dt-bindings | 1 + > > > arch/sh/kernel/setup.c | 19 ++++++++++++++++++ > > > include/dt-bindings/interrupt-controller/sh_intc.h | 2 ++ > > > 5 files changed, 36 insertions(+), 14 deletions(-) > > > create mode 120000 arch/sh/boot/dts/include/dt-bindings > > > create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h > > > > > > diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c > > > index bf3a166..9570873 100644 > > > --- a/arch/sh/boards/of-generic.c > > > +++ b/arch/sh/boards/of-generic.c > > > @@ -112,29 +112,25 @@ static int noopi(void) > > > return 0; > > > } > > > > > > -static void __init sh_of_mem_reserve(void) > > > +static void __init sh_of_mem_init(void) > > > { > > > early_init_fdt_reserve_self(); > > > early_init_fdt_scan_reserved_mem(); > > > } > > > > > > -static void __init sh_of_time_init(void) > > > -{ > > > - pr_info("SH generic board support: scanning for clocksource devices\n"); > > > - clocksource_probe(); > > > -} > > > > Why did you remove this? Without it you won't get clock > > event/clocksource devices from the device tree so the only way to have > > a working timer interrupt is if the driver is hard-coded somewhere. > > It not needed on Common Clock Framework. > tmu define in dts. It is needed. clocksources are something completely different from "clk"s. A clocksource is the modern source of time data for the kernel timekeeping system (without one, you're stuck using jiffies and very low-res time), and the probe also gets clock_event_devices which are the source of timer interrupts. Without this, unless you have a hard-coded source of timer interrupt for the board, you won't get a timer interrupt and the kernel will hang early in the boot process. > > > static void __init sh_of_setup(char **cmdline_p) > > > { > > > - unflatten_device_tree(); > > > - > > > - board_time_init = sh_of_time_init; > > > + struct device_node *cpu; > > > + int freq; > > > > > > sh_mv.mv_name = of_flat_dt_get_machine_name(); > > > if (!sh_mv.mv_name) > > > sh_mv.mv_name = "Unknown SH model"; > > > > > > sh_of_smp_probe(); > > > + cpu = of_find_node_by_name(NULL, "cpu"); > > > + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) > > > + preset_lpj = freq / 500; > > > } > > > > I setup the DT-based pseudo-board to use the generic calibrate-delay > > rather than hard-coding lpj. Ideally we could just get rid of bogomips > > completely but there are probably still some things using it. Is there > > a reason you prefer making up a value for lpj based on the cpu clock > > rate? > > clockevent initalize after calibrate delay. > So don't work interrupt based calibrate. Currently, it initializes before, but you removed the probe that initializes it (above), clocksource_probe(). > > > static int sh_of_irq_demux(int irq) > > > @@ -167,8 +163,7 @@ static struct sh_machine_vector __initmv sh_of_generic_mv = { > > > .mv_init_irq = sh_of_init_irq, > > > .mv_clk_init = sh_of_clk_init, > > > .mv_mode_pins = noopi, > > > - .mv_mem_init = noop, > > > - .mv_mem_reserve = sh_of_mem_reserve, > > > + .mv_mem_init = sh_of_mem_init, > > > > Is there a reason for this renaming? The function seems to be dealing > > purely with reserving memory ranges. > > mv_mem_reserve too late call in MMU system. OK. > > > if (!dt_virt || !early_init_dt_scan(dt_virt)) { > > > pr_crit("Error: invalid device tree blob" > > > @@ -267,8 +276,13 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > > > > > > void __init setup_arch(char **cmdline_p) > > > { > > > +#ifdef CONFIG_OF > > > + unflatten_device_tree(); > > > +#endif > > > enable_mmu(); > > > > Was this moved to setup_arch to have it before enable_mmu? I think > > that makes sense. > > early_init_dt_alloc_memory_arch used physical address. > It override on sh-specific, can after enable_mmu. > But I don't feel an advantage. I dont think we should be putting sh-specific code in early_init_dt_alloc_memory. If calling unflatten_device_tree before enable_mmu avoids the need to do that, it seems like the right choice. Is this how other archs do it? I think we should try to be as consistent as possible with general practice across the kernel. > > > +#ifndef CONFIG_OF > > > ROOT_DEV = old_decode_dev(ORIG_ROOT_DEV); > > > > > > printk(KERN_NOTICE "Boot params:\n" > > > @@ -290,6 +304,7 @@ void __init setup_arch(char **cmdline_p) > > > > > > if (!MOUNT_ROOT_RDONLY) > > > root_mountflags &= ~MS_RDONLY; > > > +#endif > > > > Do these boot params only make sense for non-DT setups? > > > > > +#if !defined(CONFIG_OF) || defined(USE_BUILTIN_DTB) > > > /* Save unparsed command line copy for /proc/cmdline */ > > > memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE); > > > *cmdline_p = command_line; > > > +#else > > > + *cmdline_p = boot_command_line; > > > +#endif > > > > I think this is wrong -- it causes the builtin command line and > > bootloader-provided command line to be ignored on DT kernels. Do you > > just want to deprecate builtin and bootloader-provided command lines? > > Or is it just a side effect of adding support for chosen/bootarg? > > Yes. I think zero-page passing only non-DT. > DT using chosen/bootargs. This precludes having the bootloader provide a DTB from ROM, since it needs to be able to patch the DTB with user preferences, and also requires DTB-patching code to be added to the bootloader. At the very least we should retain the ability to have a builtin command line in the kernel that's appended to the one from the DTB (and possibly allow the order to be changed), but I think we should also allow the bootloader to pass in a command line like in the non-DTB setup. DTB and command line are very different things (hardware description vs user preference, and OS-generic vs OS-specific) and the whole chosen/bootargs system is something of a hack. > > > parse_early_param(); > > > > > > diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h > > > new file mode 100644 > > > index 0000000..8c9dcdc > > > --- /dev/null > > > +++ b/include/dt-bindings/interrupt-controller/sh_intc.h > > > @@ -0,0 +1,2 @@ > > > +#define evt2irq(evt) (((evt) >> 5) - 16) > > > +#define irq2evt(irq) (((irq) + 16) << 5) > > > > This seems unrelated to other things in this patch. > > It using DT define. Ah, I see, it's used later in the dts itself. But why not just put the actual IRQ numbers in the dts directly? The evt numbers seem like an implementation detail, whereas device tree bindings should be stable and independent of kernel sources. If evt numbers are really what makes sense to have in the dts, then sh_intc should probably define its irq_domain so that it maps evt numbers to irq numbers. But that doesn't seem necessary. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 May 2016 01:28:07 +0900, Rich Felker wrote: > > On Tue, May 10, 2016 at 05:25:36PM +0900, Yoshinori Sato wrote: > > On Wed, 04 May 2016 12:10:05 +0900, > > Rich Felker wrote: > > > > > > On Sun, May 01, 2016 at 02:08:29PM +0900, Yoshinori Sato wrote: > > > > Changes bellow > > > > - FDT setup timing fix. > > > > - chosen/bootargs support. > > > > - zImage support. > > > > - DT binding helper macro. > > > > > > > > Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> > > > > --- > > > > arch/sh/boards/of-generic.c | 23 +++++++++++----------- > > > > arch/sh/boot/compressed/head_32.S | 5 +++-- > > > > arch/sh/boot/dts/include/dt-bindings | 1 + > > > > arch/sh/kernel/setup.c | 19 ++++++++++++++++++ > > > > include/dt-bindings/interrupt-controller/sh_intc.h | 2 ++ > > > > 5 files changed, 36 insertions(+), 14 deletions(-) > > > > create mode 120000 arch/sh/boot/dts/include/dt-bindings > > > > create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h > > > > > > > > diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c > > > > index bf3a166..9570873 100644 > > > > --- a/arch/sh/boards/of-generic.c > > > > +++ b/arch/sh/boards/of-generic.c > > > > @@ -112,29 +112,25 @@ static int noopi(void) > > > > return 0; > > > > } > > > > > > > > -static void __init sh_of_mem_reserve(void) > > > > +static void __init sh_of_mem_init(void) > > > > { > > > > early_init_fdt_reserve_self(); > > > > early_init_fdt_scan_reserved_mem(); > > > > } > > > > > > > > -static void __init sh_of_time_init(void) > > > > -{ > > > > - pr_info("SH generic board support: scanning for clocksource devices\n"); > > > > - clocksource_probe(); > > > > -} > > > > > > Why did you remove this? Without it you won't get clock > > > event/clocksource devices from the device tree so the only way to have > > > a working timer interrupt is if the driver is hard-coded somewhere. > > > > It not needed on Common Clock Framework. > > tmu define in dts. > > It is needed. clocksources are something completely different from > "clk"s. A clocksource is the modern source of time data for the kernel > timekeeping system (without one, you're stuck using jiffies and very > low-res time), and the probe also gets clock_event_devices which are > the source of timer interrupts. Without this, unless you have a > hard-coded source of timer interrupt for the board, you won't get a > timer interrupt and the kernel will hang early in the boot process. OK. > > > > { > > > > - unflatten_device_tree(); > > > > - > > > > - board_time_init = sh_of_time_init; > > > > + struct device_node *cpu; > > > > + int freq; > > > > > > > > sh_mv.mv_name = of_flat_dt_get_machine_name(); > > > > if (!sh_mv.mv_name) > > > > sh_mv.mv_name = "Unknown SH model"; > > > > > > > > sh_of_smp_probe(); > > > > + cpu = of_find_node_by_name(NULL, "cpu"); > > > > + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) > > > > + preset_lpj = freq / 500; > > > > } > > > > > > I setup the DT-based pseudo-board to use the generic calibrate-delay > > > rather than hard-coding lpj. Ideally we could just get rid of bogomips > > > completely but there are probably still some things using it. Is there > > > a reason you prefer making up a value for lpj based on the cpu clock > > > rate? > > > > clockevent initalize after calibrate delay. > > So don't work interrupt based calibrate. > > Currently, it initializes before, but you removed the probe that > initializes it (above), clocksource_probe(). OK. > > > > static int sh_of_irq_demux(int irq) > > > > @@ -167,8 +163,7 @@ static struct sh_machine_vector __initmv sh_of_generic_mv = { > > > > .mv_init_irq = sh_of_init_irq, > > > > .mv_clk_init = sh_of_clk_init, > > > > .mv_mode_pins = noopi, > > > > - .mv_mem_init = noop, > > > > - .mv_mem_reserve = sh_of_mem_reserve, > > > > + .mv_mem_init = sh_of_mem_init, > > > > > > Is there a reason for this renaming? The function seems to be dealing > > > purely with reserving memory ranges. > > > > mv_mem_reserve too late call in MMU system. > > OK. > > > > > if (!dt_virt || !early_init_dt_scan(dt_virt)) { > > > > pr_crit("Error: invalid device tree blob" > > > > @@ -267,8 +276,13 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) > > > > > > > > void __init setup_arch(char **cmdline_p) > > > > { > > > > +#ifdef CONFIG_OF > > > > + unflatten_device_tree(); > > > > +#endif > > > > enable_mmu(); > > > > > > Was this moved to setup_arch to have it before enable_mmu? I think > > > that makes sense. > > > > early_init_dt_alloc_memory_arch used physical address. > > It override on sh-specific, can after enable_mmu. > > But I don't feel an advantage. > > I dont think we should be putting sh-specific code in > early_init_dt_alloc_memory. If calling unflatten_device_tree before > enable_mmu avoids the need to do that, it seems like the right choice. > Is this how other archs do it? I think we should try to be as > consistent as possible with general practice across the kernel. OK. It's considered. > > > > +#ifndef CONFIG_OF > > > > ROOT_DEV = old_decode_dev(ORIG_ROOT_DEV); > > > > > > > > printk(KERN_NOTICE "Boot params:\n" > > > > @@ -290,6 +304,7 @@ void __init setup_arch(char **cmdline_p) > > > > > > > > if (!MOUNT_ROOT_RDONLY) > > > > root_mountflags &= ~MS_RDONLY; > > > > +#endif > > > > > > Do these boot params only make sense for non-DT setups? > > > > > > > +#if !defined(CONFIG_OF) || defined(USE_BUILTIN_DTB) > > > > /* Save unparsed command line copy for /proc/cmdline */ > > > > memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE); > > > > *cmdline_p = command_line; > > > > +#else > > > > + *cmdline_p = boot_command_line; > > > > +#endif > > > > > > I think this is wrong -- it causes the builtin command line and > > > bootloader-provided command line to be ignored on DT kernels. Do you > > > just want to deprecate builtin and bootloader-provided command lines? > > > Or is it just a side effect of adding support for chosen/bootarg? > > > > Yes. I think zero-page passing only non-DT. > > DT using chosen/bootargs. > > This precludes having the bootloader provide a DTB from ROM, since it > needs to be able to patch the DTB with user preferences, and also > requires DTB-patching code to be added to the bootloader. At the very > least we should retain the ability to have a builtin command line in > the kernel that's appended to the one from the DTB (and possibly allow > the order to be changed), but I think we should also allow the > bootloader to pass in a command line like in the non-DTB setup. > > DTB and command line are very different things (hardware description > vs user preference, and OS-generic vs OS-specific) and the whole > chosen/bootargs system is something of a hack. It's better to decide a requirement to a boot loader. u-boot is passing to bootargs in easy way. > > > > parse_early_param(); > > > > > > > > diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h > > > > new file mode 100644 > > > > index 0000000..8c9dcdc > > > > --- /dev/null > > > > +++ b/include/dt-bindings/interrupt-controller/sh_intc.h > > > > @@ -0,0 +1,2 @@ > > > > +#define evt2irq(evt) (((evt) >> 5) - 16) > > > > +#define irq2evt(irq) (((irq) + 16) << 5) > > > > > > This seems unrelated to other things in this patch. > > > > It using DT define. > > Ah, I see, it's used later in the dts itself. But why not just put the > actual IRQ numbers in the dts directly? The evt numbers seem like an > implementation detail, whereas device tree bindings should be stable > and independent of kernel sources. An IRQ is the virtual value, so it's necessary to convert from EVT. SH3/4 interrupt controller using EVT. > If evt numbers are really what makes sense to have in the dts, then > sh_intc should probably define its irq_domain so that it maps evt > numbers to irq numbers. But that doesn't seem necessary. It good way. > > Rich
diff --git a/arch/sh/boards/of-generic.c b/arch/sh/boards/of-generic.c index bf3a166..9570873 100644 --- a/arch/sh/boards/of-generic.c +++ b/arch/sh/boards/of-generic.c @@ -112,29 +112,25 @@ static int noopi(void) return 0; } -static void __init sh_of_mem_reserve(void) +static void __init sh_of_mem_init(void) { early_init_fdt_reserve_self(); early_init_fdt_scan_reserved_mem(); } -static void __init sh_of_time_init(void) -{ - pr_info("SH generic board support: scanning for clocksource devices\n"); - clocksource_probe(); -} - static void __init sh_of_setup(char **cmdline_p) { - unflatten_device_tree(); - - board_time_init = sh_of_time_init; + struct device_node *cpu; + int freq; sh_mv.mv_name = of_flat_dt_get_machine_name(); if (!sh_mv.mv_name) sh_mv.mv_name = "Unknown SH model"; sh_of_smp_probe(); + cpu = of_find_node_by_name(NULL, "cpu"); + if (!of_property_read_u32(cpu, "clock-frequency", &freq)) + preset_lpj = freq / 500; } static int sh_of_irq_demux(int irq) @@ -167,8 +163,7 @@ static struct sh_machine_vector __initmv sh_of_generic_mv = { .mv_init_irq = sh_of_init_irq, .mv_clk_init = sh_of_clk_init, .mv_mode_pins = noopi, - .mv_mem_init = noop, - .mv_mem_reserve = sh_of_mem_reserve, + .mv_mem_init = sh_of_mem_init, }; struct sh_clk_ops; @@ -194,3 +189,7 @@ static int __init sh_of_device_init(void) return 0; } arch_initcall_sync(sh_of_device_init); + +void intc_finalize(void) +{ +} diff --git a/arch/sh/boot/compressed/head_32.S b/arch/sh/boot/compressed/head_32.S index 3e15032..cd34377 100644 --- a/arch/sh/boot/compressed/head_32.S +++ b/arch/sh/boot/compressed/head_32.S @@ -14,7 +14,8 @@ startup: /* Load initial status register */ mov.l init_sr, r1 ldc r1, sr - + /* Save FDT address */ + mov r4, r13 /* Move myself to proper location if necessary */ mova 1f, r0 mov.l 1f, r2 @@ -83,7 +84,7 @@ l1: /* Jump to the start of the decompressed kernel */ mov.l kernel_start_addr, r0 jmp @r0 - nop + mov r13,r4 .align 2 bss_start_addr: diff --git a/arch/sh/boot/dts/include/dt-bindings b/arch/sh/boot/dts/include/dt-bindings new file mode 120000 index 0000000..08c00e4 --- /dev/null +++ b/arch/sh/boot/dts/include/dt-bindings @@ -0,0 +1 @@ +../../../../../include/dt-bindings \ No newline at end of file diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c index 5d34605..f6bb105 100644 --- a/arch/sh/kernel/setup.c +++ b/arch/sh/kernel/setup.c @@ -177,7 +177,12 @@ disable: #ifndef CONFIG_GENERIC_CALIBRATE_DELAY void calibrate_delay(void) { +#ifndef CONFIG_OF struct clk *clk = clk_get(NULL, "cpu_clk"); +#else + struct device_node *cpu = of_find_node_by_name(NULL, "cpu"); + struct clk *clk = of_clk_get_by_name(cpu, NULL); +#endif if (IS_ERR(clk)) panic("Need a sane CPU clock definition!"); @@ -251,7 +256,11 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) /* Avoid calling an __init function on secondary cpus. */ if (done) return; +#ifdef CONFIG_USE_BUILTIN_DTB + dt_virt = __dtb_start; +#else dt_virt = phys_to_virt(dt_phys); +#endif if (!dt_virt || !early_init_dt_scan(dt_virt)) { pr_crit("Error: invalid device tree blob" @@ -267,8 +276,13 @@ void __ref sh_fdt_init(phys_addr_t dt_phys) void __init setup_arch(char **cmdline_p) { +#ifdef CONFIG_OF + unflatten_device_tree(); +#endif enable_mmu(); + +#ifndef CONFIG_OF ROOT_DEV = old_decode_dev(ORIG_ROOT_DEV); printk(KERN_NOTICE "Boot params:\n" @@ -290,6 +304,7 @@ void __init setup_arch(char **cmdline_p) if (!MOUNT_ROOT_RDONLY) root_mountflags &= ~MS_RDONLY; +#endif init_mm.start_code = (unsigned long) _text; init_mm.end_code = (unsigned long) _etext; init_mm.end_data = (unsigned long) _edata; @@ -312,9 +327,13 @@ void __init setup_arch(char **cmdline_p) #endif #endif +#if !defined(CONFIG_OF) || defined(USE_BUILTIN_DTB) /* Save unparsed command line copy for /proc/cmdline */ memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE); *cmdline_p = command_line; +#else + *cmdline_p = boot_command_line; +#endif parse_early_param(); diff --git a/include/dt-bindings/interrupt-controller/sh_intc.h b/include/dt-bindings/interrupt-controller/sh_intc.h new file mode 100644 index 0000000..8c9dcdc --- /dev/null +++ b/include/dt-bindings/interrupt-controller/sh_intc.h @@ -0,0 +1,2 @@ +#define evt2irq(evt) (((evt) >> 5) - 16) +#define irq2evt(irq) (((irq) + 16) << 5)
Changes bellow - FDT setup timing fix. - chosen/bootargs support. - zImage support. - DT binding helper macro. Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp> --- arch/sh/boards/of-generic.c | 23 +++++++++++----------- arch/sh/boot/compressed/head_32.S | 5 +++-- arch/sh/boot/dts/include/dt-bindings | 1 + arch/sh/kernel/setup.c | 19 ++++++++++++++++++ include/dt-bindings/interrupt-controller/sh_intc.h | 2 ++ 5 files changed, 36 insertions(+), 14 deletions(-) create mode 120000 arch/sh/boot/dts/include/dt-bindings create mode 100644 include/dt-bindings/interrupt-controller/sh_intc.h