Re: [PATCH 6/7] RISC-V: arch/riscv/kernel

[Palmer Dabbelt <palmer@dabbelt.com>]

On Tue, 23 May 2017 06:35:23 PDT (-0700), Arnd Bergmann wrote:
>> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>
> Please move the majority of this file into drivers/irqchip as a
> standalone driver.

OK.

  https://github.com/riscv/riscv-linux/commit/549c7f5ef63d7be04c9cac7e332ef81ec6ffe103

>> 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@au.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]

Sounds good

  https://github.com/riscv/riscv-linux/commit/4aa540bf849b2a190e288e7d25d262dee21306b3
  https://github.com/riscv/riscv-linux/commit/bb3b4c6ca4841538d101f2b9c437f5dccda0b3a7

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

Sorry, but the only Google results for PCI_MIN_IO is this email.  There don't
appear to be any relevant references to PCI_MIN in the kernel

  $ git grep PCI_MIN_
  arch/mips/include/asm/mach-loongson64/cs5536/cs5536_pci.h:      ((PCI_MAX_LATENCY << 24) | (PCI_MIN_GRANT << 16) | \
  arch/mips/include/asm/mach-loongson64/cs5536/cs5536_pci.h:#define PCI_MIN_GRANT                 0x00
  drivers/ata/pata_hpt366.c:      pci_write_config_byte(dev, PCI_MIN_GNT, 0x08);
  drivers/ata/pata_hpt37x.c:      pci_write_config_byte(dev, PCI_MIN_GNT, 0x08);
  drivers/ata/pata_hpt3x2n.c:     pci_write_config_byte(dev, PCI_MIN_GNT, 0x08);
  drivers/ide/hpt366.c:   pci_write_config_byte(dev, PCI_MIN_GNT, 0x08);
  drivers/net/fddi/skfp/h/skfbi.h:#define PCI_MIN_GNT     0x3e    /*  8 bit       Min_Gnt */
  drivers/net/fddi/skfp/h/skfbi.h:/*      PCI_MIN_GNT     8 bit   Min_Gnt */
  include/uapi/linux/pci_regs.h:#define PCI_MIN_GNT               0x3e    /* 8 bits */

I'm afraid that I'm not sure what to do here.

>> 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.

I've addressed most of this thanks to some other code reviews, so I'm going to
drop the comments that are about things I've already fixed.

>> +// 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.

We'll do it the right way when we add IRQ affinity control.  Thanks for the
heads up!

>> 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);
> }

OK.

  https://github.com/riscv/riscv-linux/commit/5f486cb73e3a0a5a218d26781e9eae651e59203f

>> 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.

Ah, that's great: now there's almost no code left :)

  https://github.com/riscv/riscv-linux/commit/8adad12c5525a70b8837196b8f2d4ac003a7647c

>> 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@bluespec.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.

OK. https://github.com/riscv/riscv-linux/commit/ed7545c6765ba7d705e1bc6ce7b67bb7e8cb0926

>> +#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.

Yep.  It's on the list.

>> 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.

Sounds good.

  https://github.com/riscv/riscv-linux/commit/d9fcab4603c158755e19663dec0040b28ea1aad1