Re: [PATCH 1/2] New files for 0PF FPGA board.

From: Geert Uytterhoeven
Date: Fri Jun 19 2015 - 03:47:18 EST


Hi Rob,

On Thu, Jun 18, 2015 at 7:19 PM, Rob Landley <rob@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/