| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdWR-pc3rghZcyv_CNgizwddX1fqtvkDwJbtGbBnpOqnaA@mail.gmail.com>
Date: Fri, 19 Jun 2015 09:47:00 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Rob Landley <rob@...dley.net>
Cc: Linux-sh list <linux-sh@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Yoshinori Sato <ysato@...rs.sourceforge.jp>
Subject: Re: [PATCH 1/2] New files for 0PF FPGA board.
Hi Rob,
On Thu, Jun 18, 2015 at 7:19 PM, Rob Landley <rob@...dley.net> wrote:
> New files for Open Processor Foundation j2 (superh sh2 compatible open
> hardware) FPGA board, with enough drivers to boot initramfs to a shell
> prompt on serial console. See http://0pf.org for details.
Thanks for your patch!
>
> --- /dev/null 2015-06-06 04:15:40.702718005 -0500
> +++ linux/arch/sh/boards/board-0pf.c 2015-06-17 21:20:05.825356382 -0500
> @@ -0,0 +1,270 @@
> +static inline void shj_enable_irq(struct irq_data *data)
> +{
> + unsigned int irq = data->irq;
> + volatile unsigned int vui;
> +
> +// printk("%s: IRQ %d (0x%x)\n", __func__, irq, irq);
Please no C++ style comments, nor commented-out code.
> + switch (irq) {
> + case PIT_IRQ:
> + //AQ_PIO = 0x0BB;
> + /* enable, lvl 2, vector 64 */
> + AQ_SYS = (1 << 26) | /* enable PIT */
> + (0x02 << 20) | /* interrupt level 2 */
> + (PIT_IRQ << 12) | /* vector 64 */
Magic numbers need #defines.
> + 1; /* turn off interval timer */
> + break;
> +
> + case Irq_UART0:
> + vui = *(volatile unsigned int *)(sys_SYS_BASE + Sys_IntPri);
volatile considered harmful, writel()? (more below)
> +static void __init shj_irq_init(void)
> +{
> + int c;
unsigned int
> +
> + printk(KERN_INFO "0PF FPGA interrupt controller...\n");
pr_info()
> +
> + for (c = 0; c < NR_IRQS; c++) {
> + //irq_desc[c].action = NULL;
> + //irq_desc[c].depth = 1;
> + irq_set_chip_and_handler_name(c, &shj_irq_chip,
> + handle_simple_irq, "simple");
> + }
> +}
> +
> +#ifdef CONFIG_ARCH_USES_GETTIMEOFFSET
> +// Commit 7b1f62076 switched this to a pointer
Please don't refer to git commits in comments.
> +static struct sh_machine_vector mv_se __initmv = {
> + .mv_name = "0PF_FPGA",
> + //.mv_nr_irqs = 256,
No commented-out code/data
> +static void __init start_pit(void)
> +{
> + if (request_irq
> + (PIT_IRQ, timer_interrupt, IRQF_TIMER, "pit", NULL))
> + printk("irq_desc[%p] : fail to register\n", &irq_desc[PIT_IRQ]);
pr_err()
> --- /dev/null 2015-06-06 04:15:40.702718005 -0500
> +++ linux/arch/sh/include/asm/board-0pf.h 2015-06-17 21:20:05.825356382 -0500
> @@ -0,0 +1,247 @@
> +#ifndef SGM_BOARD_H
> +#define SGM_BOARD_H
Strange include guard name, not matching file name.
> +#define sys_IntTable (*(unsigned*)0x0)
NULL? Besides, it seems unused?
> +#define SIC_ENMI ((unsigned int) 0x1<<31) /* Emulate NMI */
"BIT(31)" (more below)
> +#if 0
No #ifdef'ed-out definitions please
> --- /dev/null 2015-06-06 04:15:40.702718005 -0500
> +++ linux/arch/sh/kernel/cpu/sh2/clock-0pf.c 2015-06-17 21:20:05.825356382 -0500
> @@ -0,0 +1,80 @@
> +int __init arch_clk_init()
> +{
> + int ret;
> +
> + printk("%s(): 0PF Clock init...\n", __func__);
pr_debug()?
> --- /dev/null 2015-06-06 04:15:40.702718005 -0500
> +++ linux/arch/sh/kernel/cpu/sh2/setup-0pf.c 2015-06-17 21:20:05.825356382 -0500
> @@ -0,0 +1,82 @@
> +#if defined(CONFIG_SERIAL_UARTLITE_0PF)
> +static struct resource shj_uartlite_resources[] = {
> + [0] = DEFINE_RES_MEM(0xABCD0100, 16),
> + [1] = DEFINE_RES_IRQ(0x12),
> +
> + [2] = DEFINE_RES_MEM(0xABCD0300, 16),
> + [3] = DEFINE_RES_IRQ(0x17),
> +
> + [4] = DEFINE_RES_MEM(0xABCD0400, 16),
> + [5] = DEFINE_RES_IRQ(0x13),
> +};
> +
> +static struct platform_device shj_uartlite_device[] = {
> + [0] = { .name = "uartlite", .id = 0 },
> + [1] = { .name = "uartlite", .id = 1 },
> + [2] = { .name = "uartlite", .id = 2 },
> +};
Typically we have only the driver depend on CONFIG_XXX, not the
platform code that creates the platform device.
> +#endif
> +
> +/*****************************************************************************
> + * 0PF FPGA platform devices
> + ****************************************************************************/
> +static struct platform_device *shj_devices[] __initdata = {
> +#if defined(CONFIG_SERIAL_UARTLITE_0PF)
Idem ditto
> + shj_uartlite_device,
> + shj_uartlite_device + 1,
> + shj_uartlite_device + 2,
> +#endif
> +};
> +
> +static int __init shj_devices_setup(void)
> +{
> + int i;
> + pr_info("%s(): registering device resources\n", __func__);
> +
> +#if defined(CONFIG_SERIAL_UARTLITE_0PF)
idem ditto
> + for (i = 0; i < ARRAY_SIZE(shj_uartlite_device); i++) {
> + printk("Register UARTLITE resources %d\n", i);
> + if (platform_device_add_resources(
> + shj_uartlite_device + i,
> + shj_uartlite_resources + 2 * i,
> + 2))
> + pr_err("Failed to set uartlite %d IRQ and MEM\n", i);
> +
> + }
> +#endif
> + platform_add_devices(shj_devices, ARRAY_SIZE(shj_devices));
> +
> + return 0;
> +}
> --- /dev/null 2015-06-06 04:15:40.702718005 -0500
> +++ linux/arch/sh/configs/0pf_defconfig 2015-06-17 21:47:46.869366542 -0500
> @@ -0,0 +1,945 @@
I may be mistaken, but this doesn't look like a minimal defconfig as
created by "make savedefconfig".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists