Message-ID: <CAK8P3a0GcwER=bzYT9fuLPO4170tL2ezPZ9c=j8D3ftZWSuywA@mail.gmail.com>
Date:   Tue, 23 May 2017 15:35:23 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Palmer Dabbelt <palmer@...belt.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Olof Johansson <olof@...om.net>, albert@...ive.com
Subject: Re: [PATCH 6/7] RISC-V: arch/riscv/kernel

> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);

Please move the majority of this file into drivers/irqchip as a
standalone driver.

> diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c
> new file mode 100644
> index 000000000000..4191a5ffdd67
> --- /dev/null
> +++ b/arch/riscv/kernel/pci.c
> @@ -0,0 +1,36 @@
> +/*
> + * Code borrowed from arch/arm64/kernel/pci.c
> + *
> + * Copyright (C) 2003 Anton Blanchard <anton@...ibm.com>, IBM
> + * Copyright (C) 2014 ARM Ltd.
> + * Copyright (C) 2017 SiFive
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <linux/pci.h>
> +
> +/*
> + * Called after each bus is probed, but before its children are examined
> + */
> +void pcibios_fixup_bus(struct pci_bus *bus)
> +{
> +       /* nothing to do, expected to be removed in the future */
> +}
> +/*
> + * We don't have to worry about legacy ISA devices, so nothing to do here
> + */
> +resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> +                               resource_size_t size, resource_size_t align)
> +{
> +       return res->start;
> +}

Can you add a patch to remove the need for this, and send that to the
PCI maintainers?

In the long run, I think we want both of these to be pci host bridge
driver specific callbacks rather than per-architecture definitions, but
for the moment, moving the empty version as a __weak copy
into drivers/pci/ should be sufficient.

[note: don't ever use __weak elsewhere, the use in PCI is
 only done for historic reasons and we want to get rid of that
 too, but for now it's more important to avoid adding yet another
 pointless copy]

If you don't care about LPC/ISA devices, then your PCI_MIN_IO
should also be zero instead of 0x1000

> diff --git a/arch/riscv/kernel/plic.c b/arch/riscv/kernel/plic.c
> new file mode 100644
> index 000000000000..5b3d4241f4e2
> --- /dev/null
> +++ b/arch/riscv/kernel/plic.c

drivers/irqchip/riscv-plic.c

The file needs some work for following coding style, once that
is done, please submit to the irqchip maintainers.

> +#define PLIC_HART_CONTEXT(data, i)     (struct plic_hart_context *)((char*)data->reg + HART_BASE + HART_SIZE*i)
> +#define PLIC_ENABLE_CONTEXT(data, i)   (struct plic_enable_context *)((char*)data->reg + ENABLE_BASE + ENABLE_SIZE*i)
> +#define PLIC_PRIORITY(data)            (struct plic_priority *)((char *)data->reg + PRIORITY_BASE)
> +
> +struct plic_hart_context {
> +       volatile u32 threshold;
> +       volatile u32 claim;
> +};
> +
> +struct plic_enable_context {
> +       atomic_t mask[32]; // 32-bit * 32-entry
> +};
> +
> +struct plic_priority {
> +       volatile u32 prio[MAX_DEVICES];
> +};

The 'volatile' seems misplaced here. What is it for?

> +// TODO: add a /sys interface to set priority + per-hart enables for steering

No driver-private sysfs interfaces please for irqchips please.
See http://elixir.free-electrons.com/linux/latest/source/Documentation/IRQ-affinity.txt
for setting the affinity.

> diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
> new file mode 100644
> index 000000000000..58bad9598e21
> --- /dev/null
> +++ b/arch/riscv/kernel/reset.c
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + */
> +
> +#include <linux/reboot.h>
> +#include <linux/export.h>
> +#include <asm/sbi.h>
> +
> +void (*pm_power_off)(void) = machine_power_off;
> +EXPORT_SYMBOL(pm_power_off);
> +
> +void machine_restart(char *cmd)
> +{
> +}

Call do_kernel_restart(cmd) here.

> +void machine_halt(void)
> +{
> +}

This should not return. Either make it call sbi_shutdown as well,
or use the ARM implementation:

void machine_halt(void)
{
        local_irq_disable();
        smp_send_stop();
        while (1);
}

> diff --git a/arch/riscv/kernel/sbi-con.c b/arch/riscv/kernel/sbi-con.c
> new file mode 100644
> index 000000000000..86baeb5ef0cd
> --- /dev/null
> +++ b/arch/riscv/kernel/sbi-con.c

As Olof said, move it to drivers/tty/hvc/ and use those helpers.

> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> new file mode 100644
> index 000000000000..3e07308e24f5
> --- /dev/null
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2014 Darius Rad <darius@...espec.com>
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + *   NON INFRINGEMENT.  See the GNU General Public License for
> + *   more details.
> + */
> +
> +#include <linux/syscalls.h>
> +#include <asm/unistd.h>
> +
> +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> +       unsigned long, prot, unsigned long, flags,
> +       unsigned long, fd, off_t, offset)
> +{
> +       if (unlikely(offset & (~PAGE_MASK)))
> +               return -EINVAL;
> +       return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> +}
> +
> +#ifndef CONFIG_64BIT
> +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> +       unsigned long, prot, unsigned long, flags,
> +       unsigned long, fd, off_t, offset)
> +{
> +       /* Note that the shift for mmap2 is constant (12),
> +          regardless of PAGE_SIZE */
> +       if (unlikely(offset & (~PAGE_MASK >> 12)))
> +               return -EINVAL;
> +       return sys_mmap_pgoff(addr, len, prot, flags, fd,
> +               offset >> (PAGE_SHIFT - 12));
> +}
> +#endif /* !CONFIG_64BIT */

The first one should be CONFIG_64BIT only.

> +#ifdef CONFIG_RV_SYSRISCV_ATOMIC
> +SYSCALL_DEFINE4(sysriscv, unsigned long, cmd, unsigned long, arg1,
> +       unsigned long, arg2, unsigned long, arg3)
> +{
> +       unsigned long flags;
> +       unsigned long prev;
> +       unsigned int *ptr;
> +       unsigned int err;
> +
> +       switch (cmd) {
> +       case RISCV_ATOMIC_CMPXCHG:
> +               ptr = (unsigned int *)arg1;
> +               if (!access_ok(VERIFY_WRITE, ptr, sizeof(unsigned int)))
> +                       return -EFAULT;
> +
> +               preempt_disable();
> +               raw_local_irq_save(flags);
> +               err = __get_user(prev, ptr);
> +               if (likely(!err && prev == arg2))
> +                       err = __put_user(arg3, ptr);
> +               raw_local_irq_restore(flags);
> +               preempt_enable();
> +
> +               return unlikely(err) ? err : prev;
> +
> +       case RISCV_ATOMIC_CMPXCHG64:

Make these two separate syscalls and get rid of the wrapper
(I already mentioned it in the header file comments, but it
fits better here).

It may be good to have an optimized version in the vdso
that does an atomic operation directly if the CPU supports
it.

> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> new file mode 100644
> index 000000000000..ce8c459fadaa
> --- /dev/null
> +++ b/arch/riscv/kernel/time.c

drivers/clocksource/riscv-timer.c, and submit it to the
respective maintainers.

      Arnd

Powered by blists - more mailing lists