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

2017-06-06 Thread Palmer Dabbelt 
On Tue, 06 Jun 2017 02:03:51 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt  wrote:
>> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
 On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>>>
  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also 
 present in
other architectures.
>>>
>>> What is the warning here? I would assume that you should leave this
>>> unchanged as well.
>>
>> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
>> variables or standard guards instead
>> #2533: FILE: arch/riscv/include/asm/elf.h:79:
>> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES
>
> Ok, you can definitely ignore this one. The warning is meant to prevent adding
> new macros like that, but the macro already exists in the other architectures,
> and I see no point in converting them all into a CONFIG_ symbol.

Sounds good.

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

2017-06-06 Thread Palmer Dabbelt 
On Tue, 06 Jun 2017 02:03:51 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt  wrote:
>> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
 On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>>>
  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also 
 present in
other architectures.
>>>
>>> What is the warning here? I would assume that you should leave this
>>> unchanged as well.
>>
>> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
>> variables or standard guards instead
>> #2533: FILE: arch/riscv/include/asm/elf.h:79:
>> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES
>
> Ok, you can definitely ignore this one. The warning is meant to prevent adding
> new macros like that, but the macro already exists in the other architectures,
> and I see no point in converting them all into a CONFIG_ symbol.

Sounds good.

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

2017-06-06 Thread Palmer Dabbelt 
On Tue, 06 Jun 2017 02:01:23 PDT (-0700), Arnd Bergmann wrote:
> On Sat, Jun 3, 2017 at 1:56 AM, Palmer Dabbelt  wrote:
>> On Tue, 23 May 2017 06:35:23 PDT (-0700), Arnd Bergmann wrote:
 +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>>> 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.
>
> Sorry, I meant PCIBIOS_MIN_IO

OK, fixed.

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

2017-06-06 Thread Palmer Dabbelt 
On Tue, 06 Jun 2017 02:01:23 PDT (-0700), Arnd Bergmann wrote:
> On Sat, Jun 3, 2017 at 1:56 AM, Palmer Dabbelt  wrote:
>> On Tue, 23 May 2017 06:35:23 PDT (-0700), Arnd Bergmann wrote:
 +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>>> 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.
>
> Sorry, I meant PCIBIOS_MIN_IO

OK, fixed.

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

2017-06-06 Thread Arnd Bergmann 
On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt  wrote:
> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
>>> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>>
>>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present 
>>> in
>>>other architectures.
>>
>> What is the warning here? I would assume that you should leave this
>> unchanged as well.
>
> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
> variables or standard guards instead
> #2533: FILE: arch/riscv/include/asm/elf.h:79:
> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

Ok, you can definitely ignore this one. The warning is meant to prevent adding
new macros like that, but the macro already exists in the other architectures,
and I see no point in converting them all into a CONFIG_ symbol.

   Arnd

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

2017-06-06 Thread Arnd Bergmann 
On Tue, Jun 6, 2017 at 6:56 AM, Palmer Dabbelt  wrote:
> On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
>> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
>>> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>>
>>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present 
>>> in
>>>other architectures.
>>
>> What is the warning here? I would assume that you should leave this
>> unchanged as well.
>
> ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
> variables or standard guards instead
> #2533: FILE: arch/riscv/include/asm/elf.h:79:
> +#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

Ok, you can definitely ignore this one. The warning is meant to prevent adding
new macros like that, but the macro already exists in the other architectures,
and I see no point in converting them all into a CONFIG_ symbol.

   Arnd

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

2017-06-06 Thread Arnd Bergmann 
On Sat, Jun 3, 2017 at 1:56 AM, Palmer Dabbelt  wrote:
> On Tue, 23 May 2017 06:35:23 PDT (-0700), Arnd Bergmann wrote:
>>> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>> 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.

Sorry, I meant PCIBIOS_MIN_IO

   Arnd

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

2017-06-06 Thread Arnd Bergmann 
On Sat, Jun 3, 2017 at 1:56 AM, Palmer Dabbelt  wrote:
> On Tue, 23 May 2017 06:35:23 PDT (-0700), Arnd Bergmann wrote:
>>> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
>> 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.

Sorry, I meant PCIBIOS_MIN_IO

   Arnd

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

2017-06-05 Thread Palmer Dabbelt 
On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
>> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>
>>  * Parens around single-statement __asm__ macros.  For these I also get a
>>message when they're wrapped in "do {} while (0)", so I'm not sure what 
>> else
>>to do.
>
> I would generally recommend using inline functions for those, and only do
> macros when you need them.

We've tried to avoid new macros, so these are mostly from places where other
architectures use macros (like mb) or where we need to use CPP token pasting
(like __op_bit).  Should I change things like mb?

>>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>>parens because they go directly to the assembler, so I'm considering this 
>> a
>>false-positive.
>
> agreed
>
>>  * Warnings about volatile in function declarations in bitops.h.  These are
>>copied from other architectures.  There were a handful of other volatiles
>>that I fixed,, but I think these should stay.
>
> Agreed, bitops.h is one of the few headers that should use 'volatile'.
>
>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present 
>> in
>>other architectures.
>
> What is the warning here? I would assume that you should leave this
> unchanged as well.

ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
variables or standard guards instead
#2533: FILE: arch/riscv/include/asm/elf.h:79:
+#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

>>  * We added new typedefs, I can remove these if that's a problem.  They're
>>there to match our other code (bootloader and simulator).
>
> It depends. What typedefs are those? Removing the typedefs in both
> the kernel and the other code that uses the same types is likely the
> best option here.

OK, I'll add it to my TODO list.

>>  * A handful of lines over 80 characters that I think are onerous to break 
>> any
>>more.
>
> Right, don't worry about it too much, and use common sense for this
> warning.
>
>>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>>of these, but the remaining ones I don't know how to fix (in show_regs, 
>> for
>>example, where arm64 also has them).
>
> KERN_CONT

https://github.com/riscv/riscv-linux/commit/98e8fe9cb19d495180a9be03a0aa48c0183dd5be

>>  * Extern declarations in C files, all of which link to symbols in assembly 
>> or
>>linker scripts.  These were copied from other architectures.
>
> I would try to fix those by using a header even if there is only one user.
> I'd actually like to get a compile-time warning for those in the long run,
> maybe with 'make W=1', so better don't introduce new ones.

OK, I'll fix them.

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

2017-06-05 Thread Palmer Dabbelt 
On Thu, 25 May 2017 12:51:54 PDT (-0700), Arnd Bergmann wrote:
> On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
>> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
>
>>  * Parens around single-statement __asm__ macros.  For these I also get a
>>message when they're wrapped in "do {} while (0)", so I'm not sure what 
>> else
>>to do.
>
> I would generally recommend using inline functions for those, and only do
> macros when you need them.

We've tried to avoid new macros, so these are mostly from places where other
architectures use macros (like mb) or where we need to use CPP token pasting
(like __op_bit).  Should I change things like mb?

>>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>>parens because they go directly to the assembler, so I'm considering this 
>> a
>>false-positive.
>
> agreed
>
>>  * Warnings about volatile in function declarations in bitops.h.  These are
>>copied from other architectures.  There were a handful of other volatiles
>>that I fixed,, but I think these should stay.
>
> Agreed, bitops.h is one of the few headers that should use 'volatile'.
>
>>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present 
>> in
>>other architectures.
>
> What is the warning here? I would assume that you should leave this
> unchanged as well.

ERROR: #define of 'ARCH_HAS_SETUP_ADDITIONAL_PAGES' is wrong - use Kconfig 
variables or standard guards instead
#2533: FILE: arch/riscv/include/asm/elf.h:79:
+#define ARCH_HAS_SETUP_ADDITIONAL_PAGES

>>  * We added new typedefs, I can remove these if that's a problem.  They're
>>there to match our other code (bootloader and simulator).
>
> It depends. What typedefs are those? Removing the typedefs in both
> the kernel and the other code that uses the same types is likely the
> best option here.

OK, I'll add it to my TODO list.

>>  * A handful of lines over 80 characters that I think are onerous to break 
>> any
>>more.
>
> Right, don't worry about it too much, and use common sense for this
> warning.
>
>>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>>of these, but the remaining ones I don't know how to fix (in show_regs, 
>> for
>>example, where arm64 also has them).
>
> KERN_CONT

https://github.com/riscv/riscv-linux/commit/98e8fe9cb19d495180a9be03a0aa48c0183dd5be

>>  * Extern declarations in C files, all of which link to symbols in assembly 
>> or
>>linker scripts.  These were copied from other architectures.
>
> I would try to fix those by using a header even if there is only one user.
> I'd actually like to get a compile-time warning for those in the long run,
> maybe with 'make W=1', so better don't introduce new ones.

OK, I'll fix them.

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

2017-06-02 Thread Palmer Dabbelt 
On Thu, 25 May 2017 10:05:05 PDT (-0700), pa...@ucw.cz wrote:
>> +static void ci_leaf_init(struct cacheinfo *this_leaf,
>> + struct device_node *node,
>> + enum cache_type type, unsigned int level)
>> +{
>> +this_leaf->of_node = node;
>> +this_leaf->level = level;
>> +this_leaf->type = type;
>> +this_leaf->physical_line_partition = 1; // not a sector cache
>> +this_leaf->attributes = CACHE_WRITE_BACK | CACHE_READ_ALLOCATE | 
>> CACHE_WRITE_ALLOCATE; // TODO: add to DTS
>> +}
>
> You may want to run the patches through checkpatch. (Comment style,
> long lines).

Thanks.  Someone else suggested this and I've fixed most of the errors.  I'll
submit a v2 with everyone's feedback once I get through my mail.

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

2017-06-02 Thread Palmer Dabbelt 
On Thu, 25 May 2017 10:05:05 PDT (-0700), pa...@ucw.cz wrote:
>> +static void ci_leaf_init(struct cacheinfo *this_leaf,
>> + struct device_node *node,
>> + enum cache_type type, unsigned int level)
>> +{
>> +this_leaf->of_node = node;
>> +this_leaf->level = level;
>> +this_leaf->type = type;
>> +this_leaf->physical_line_partition = 1; // not a sector cache
>> +this_leaf->attributes = CACHE_WRITE_BACK | CACHE_READ_ALLOCATE | 
>> CACHE_WRITE_ALLOCATE; // TODO: add to DTS
>> +}
>
> You may want to run the patches through checkpatch. (Comment style,
> long lines).

Thanks.  Someone else suggested this and I've fixed most of the errors.  I'll
submit a v2 with everyone's feedback once I get through my mail.

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

2017-06-02 Thread Palmer Dabbelt 
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 ..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 , 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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * 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 ..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 ..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

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

2017-06-02 Thread Palmer Dabbelt 
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 ..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 , 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 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * 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 ..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 ..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

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

2017-05-25 Thread Arnd Bergmann 
On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:

>  * Parens around single-statement __asm__ macros.  For these I also get a
>message when they're wrapped in "do {} while (0)", so I'm not sure what 
> else
>to do.

I would generally recommend using inline functions for those, and only do
macros when you need them.

>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>parens because they go directly to the assembler, so I'm considering this a
>false-positive.

agreed

>  * Warnings about volatile in function declarations in bitops.h.  These are
>copied from other architectures.  There were a handful of other volatiles
>that I fixed,, but I think these should stay.

Agreed, bitops.h is one of the few headers that should use 'volatile'.

>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>other architectures.

What is the warning here? I would assume that you should leave this
unchanged as well.

>  * We added new typedefs, I can remove these if that's a problem.  They're
>there to match our other code (bootloader and simulator).

It depends. What typedefs are those? Removing the typedefs in both
the kernel and the other code that uses the same types is likely the
best option here.

>  * A handful of lines over 80 characters that I think are onerous to break any
>more.

Right, don't worry about it too much, and use common sense for this
warning.

>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>of these, but the remaining ones I don't know how to fix (in show_regs, for
>example, where arm64 also has them).

KERN_CONT

>  * Extern declarations in C files, all of which link to symbols in assembly or
>linker scripts.  These were copied from other architectures.

I would try to fix those by using a header even if there is only one user.
I'd actually like to get a compile-time warning for those in the long run,
maybe with 'make W=1', so better don't introduce new ones.

 Arnd

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

2017-05-25 Thread Arnd Bergmann 
On Thu, May 25, 2017 at 3:59 AM, Palmer Dabbelt  wrote:
> On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:

>  * Parens around single-statement __asm__ macros.  For these I also get a
>message when they're wrapped in "do {} while (0)", so I'm not sure what 
> else
>to do.

I would generally recommend using inline functions for those, and only do
macros when you need them.

>  * Parens around macros like "#define RISCV_PTR .dword".  These can't have
>parens because they go directly to the assembler, so I'm considering this a
>false-positive.

agreed

>  * Warnings about volatile in function declarations in bitops.h.  These are
>copied from other architectures.  There were a handful of other volatiles
>that I fixed,, but I think these should stay.

Agreed, bitops.h is one of the few headers that should use 'volatile'.

>  * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
>other architectures.

What is the warning here? I would assume that you should leave this
unchanged as well.

>  * We added new typedefs, I can remove these if that's a problem.  They're
>there to match our other code (bootloader and simulator).

It depends. What typedefs are those? Removing the typedefs in both
the kernel and the other code that uses the same types is likely the
best option here.

>  * A handful of lines over 80 characters that I think are onerous to break any
>more.

Right, don't worry about it too much, and use common sense for this
warning.

>  * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
>of these, but the remaining ones I don't know how to fix (in show_regs, for
>example, where arm64 also has them).

KERN_CONT

>  * Extern declarations in C files, all of which link to symbols in assembly or
>linker scripts.  These were copied from other architectures.

I would try to fix those by using a header even if there is only one user.
I'd actually like to get a compile-time warning for those in the long run,
maybe with 'make W=1', so better don't introduce new ones.

 Arnd

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

2017-05-25 Thread Pavel Machek 
Hi!

> +static void ci_leaf_init(struct cacheinfo *this_leaf,
> + struct device_node *node,
> + enum cache_type type, unsigned int level)
> +{
> +this_leaf->of_node = node;
> +this_leaf->level = level;
> +this_leaf->type = type;
> +this_leaf->physical_line_partition = 1; // not a sector cache
> +this_leaf->attributes = CACHE_WRITE_BACK | CACHE_READ_ALLOCATE | 
> CACHE_WRITE_ALLOCATE; // TODO: add to DTS
> +}

You may want to run the patches through checkpatch. (Comment style,
long lines).

> +static int __populate_cache_leaves(unsigned int cpu)
> +{
> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> + struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> + struct device_node *np = of_cpu_device_node_get(cpu);
> + int levels = 1, level = 1;
> +
> + if (of_property_read_bool(np, "cache-size"))   
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> + if (of_property_read_bool(np, "i-cache-size")) 
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> + if (of_property_read_bool(np, "d-cache-size")) 
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

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

2017-05-25 Thread Pavel Machek 
Hi!

> +static void ci_leaf_init(struct cacheinfo *this_leaf,
> + struct device_node *node,
> + enum cache_type type, unsigned int level)
> +{
> +this_leaf->of_node = node;
> +this_leaf->level = level;
> +this_leaf->type = type;
> +this_leaf->physical_line_partition = 1; // not a sector cache
> +this_leaf->attributes = CACHE_WRITE_BACK | CACHE_READ_ALLOCATE | 
> CACHE_WRITE_ALLOCATE; // TODO: add to DTS
> +}

You may want to run the patches through checkpatch. (Comment style,
long lines).

> +static int __populate_cache_leaves(unsigned int cpu)
> +{
> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> + struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> + struct device_node *np = of_cpu_device_node_get(cpu);
> + int levels = 1, level = 1;
> +
> + if (of_property_read_bool(np, "cache-size"))   
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
> + if (of_property_read_bool(np, "i-cache-size")) 
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
> + if (of_property_read_bool(np, "d-cache-size")) 
> ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

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

2017-05-24 Thread Palmer Dabbelt 
On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt  wrote:
> What's missing from this patchset (ideally) is a good writeup under
> DOcumentation/ on expectations of system state (and/or configuration)
> upon entry of the kernel. For comparison, see the arm64 documentation
> where they were quite specific in this.

We don't have a spec written for this yet, but one is being written.  Is it OK
if I wait until there's a spec?

> This patch is also pushing size limits, and is getting unwieldy to
> comment on. I'll point out a few things below with plenty of snipped
> out lines.

I'm also going to start dropping diffs, as this is very big.

>> +++ b/arch/riscv/kernel/asm-offsets.c
>> @@ -0,0 +1,113 @@
>> +/*
>> + * 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.
>
> Hmm, I haven't seen these terms used often, but they seem to exist
> around the tree in a few places. arch/tile is littered with them.
>
> I am not a lawyer, but I can't seem any reference to "good title" in
> the GPLv2 text.
>
> Rather than having to go through the process of figuring out if this
> license header is acceptable or not, you might find it easier to just
> go with something more established.

This almost certainly came from Tilera: I stole our ptrace from there becuase
that was the ISA I understood best, and that header must have proliferated
everywhere else.

I've changed it to a header I copied from ARM

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

>> +static int __populate_cache_leaves(unsigned int cpu)
>> +{
>> +   struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> +   struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>> +   struct device_node *np = of_cpu_device_node_get(cpu);
>> +   int levels = 1, level = 1;
>> +
>> +   if (of_property_read_bool(np, "cache-size"))   
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
>> +   if (of_property_read_bool(np, "i-cache-size")) 
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
>> +   if (of_property_read_bool(np, "d-cache-size")) 
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
>
> Please run checkpatch, kernel coding style doesn't use one-line ifs
> (here nor elsewhere).

I went through and fixed many of the checkpatch messages.  The ones that are
left fall into the following categories:

 * Lots of uses of BUG/BUG_ON instead of WARN_ON.  Lots of these are in boot
   code, but some of them can probably be fixed.

 * Parens around single-statement __asm__ macros.  For these I also get a
   message when they're wrapped in "do {} while (0)", so I'm not sure what else
   to do.

 * Parens around macros like "#define RISCV_PTR .dword".  These can't have
   parens because they go directly to the assembler, so I'm considering this a
   false-positive.

 * Warnings about volatile in function declarations in bitops.h.  These are
   copied from other architectures.  There were a handful of other volatiles
   that I fixed,, but I think these should stay.

 * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
   other architectures.

 * We added new typedefs, I can remove these if that's a problem.  They're
   there to match our other code (bootloader and simulator).

 * A handful of lines over 80 characters that I think are onerous to break any
   more.

 * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
   of these, but the remaining ones I don't know how to fix (in show_regs, for
   example, where arm64 also has them).

 * Extern declarations in C files, all of which link to symbols in assembly or
   linker scripts.  These were copied from other architectures.

There's also a bunch of false positives:

 * The spelling of SEPC, which is correct (Supervisor Exception Program
   Counter).

 * Fall-through warnings, probably getting confused by the break looking like
   "break; \" (they're in macros).

I'll make another pass on these before a v2 patch set.

>> +/* Return -1 if not a valid hart */
>> +int riscv_of_processor_hart(struct device_node *node)
>> +{
>> +   const char *isa, *status;
>> +   u32 hart;
>> +
>> +   if (!of_device_is_compatible(node, "riscv")) return -1;
>> +   if (of_property_read_u32(node, "reg", ) || hart >= NR_CPUS) 
>> return -1;
>> +   if (of_property_read_string(node,

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

2017-05-24 Thread Palmer Dabbelt 
On Mon, 22 May 2017 19:11:35 PDT (-0700), o...@lixom.net wrote:
> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt  wrote:
> What's missing from this patchset (ideally) is a good writeup under
> DOcumentation/ on expectations of system state (and/or configuration)
> upon entry of the kernel. For comparison, see the arm64 documentation
> where they were quite specific in this.

We don't have a spec written for this yet, but one is being written.  Is it OK
if I wait until there's a spec?

> This patch is also pushing size limits, and is getting unwieldy to
> comment on. I'll point out a few things below with plenty of snipped
> out lines.

I'm also going to start dropping diffs, as this is very big.

>> +++ b/arch/riscv/kernel/asm-offsets.c
>> @@ -0,0 +1,113 @@
>> +/*
>> + * 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.
>
> Hmm, I haven't seen these terms used often, but they seem to exist
> around the tree in a few places. arch/tile is littered with them.
>
> I am not a lawyer, but I can't seem any reference to "good title" in
> the GPLv2 text.
>
> Rather than having to go through the process of figuring out if this
> license header is acceptable or not, you might find it easier to just
> go with something more established.

This almost certainly came from Tilera: I stole our ptrace from there becuase
that was the ISA I understood best, and that header must have proliferated
everywhere else.

I've changed it to a header I copied from ARM

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

>> +static int __populate_cache_leaves(unsigned int cpu)
>> +{
>> +   struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> +   struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>> +   struct device_node *np = of_cpu_device_node_get(cpu);
>> +   int levels = 1, level = 1;
>> +
>> +   if (of_property_read_bool(np, "cache-size"))   
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_UNIFIED, level);
>> +   if (of_property_read_bool(np, "i-cache-size")) 
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_INST, level);
>> +   if (of_property_read_bool(np, "d-cache-size")) 
>> ci_leaf_init(this_leaf++, np, CACHE_TYPE_DATA, level);
>
> Please run checkpatch, kernel coding style doesn't use one-line ifs
> (here nor elsewhere).

I went through and fixed many of the checkpatch messages.  The ones that are
left fall into the following categories:

 * Lots of uses of BUG/BUG_ON instead of WARN_ON.  Lots of these are in boot
   code, but some of them can probably be fixed.

 * Parens around single-statement __asm__ macros.  For these I also get a
   message when they're wrapped in "do {} while (0)", so I'm not sure what else
   to do.

 * Parens around macros like "#define RISCV_PTR .dword".  These can't have
   parens because they go directly to the assembler, so I'm considering this a
   false-positive.

 * Warnings about volatile in function declarations in bitops.h.  These are
   copied from other architectures.  There were a handful of other volatiles
   that I fixed,, but I think these should stay.

 * Definitions like ARCH_HAS_SETUP_ADDITIONAL_PAGES, these are also present in
   other architectures.

 * We added new typedefs, I can remove these if that's a problem.  They're
   there to match our other code (bootloader and simulator).

 * A handful of lines over 80 characters that I think are onerous to break any
   more.

 * Some warnings about printk() not having a KERN_ prefix.  I fixed a handful
   of these, but the remaining ones I don't know how to fix (in show_regs, for
   example, where arm64 also has them).

 * Extern declarations in C files, all of which link to symbols in assembly or
   linker scripts.  These were copied from other architectures.

There's also a bunch of false positives:

 * The spelling of SEPC, which is correct (Supervisor Exception Program
   Counter).

 * Fall-through warnings, probably getting confused by the break looking like
   "break; \" (they're in macros).

I'll make another pass on these before a v2 patch set.

>> +/* Return -1 if not a valid hart */
>> +int riscv_of_processor_hart(struct device_node *node)
>> +{
>> +   const char *isa, *status;
>> +   u32 hart;
>> +
>> +   if (!of_device_is_compatible(node, "riscv")) return -1;
>> +   if (of_property_read_u32(node, "reg", ) || hart >= NR_CPUS) 
>> return -1;
>> +   if (of_property_read_string(node, "status", ) || 
>>

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

2017-05-23 Thread Arnd Bergmann 
> +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 ..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 , 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * 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 ..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 ..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 
> +#include 
> +#include 
> +
> +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 ..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 ..3e07308e24f5
> --- /dev/null
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of

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

2017-05-23 Thread Arnd Bergmann 
> +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 ..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 , 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * 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 ..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 ..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 
> +#include 
> +#include 
> +
> +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 ..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 ..3e07308e24f5
> --- /dev/null
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + *

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

2017-05-22 Thread Olof Johansson 
On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt  wrote:
> ---
>  arch/riscv/kernel/Makefile |  19 ++
>  arch/riscv/kernel/asm-offsets.c| 113 ++
>  arch/riscv/kernel/cacheinfo.c  |  82 
>  arch/riscv/kernel/cpu.c|  81 
>  arch/riscv/kernel/entry.S  | 414 
> +
>  arch/riscv/kernel/head.S   | 139 +
>  arch/riscv/kernel/irq.c| 205 ++
>  arch/riscv/kernel/module.c | 185 +
>  arch/riscv/kernel/pci.c|  36 
>  arch/riscv/kernel/plic.c   | 208 +++
>  arch/riscv/kernel/process.c| 130 
>  arch/riscv/kernel/ptrace.c | 148 +
>  arch/riscv/kernel/reset.c  |  33 +++
>  arch/riscv/kernel/riscv_ksyms.c|  16 ++
>  arch/riscv/kernel/sbi-con.c| 214 +++
>  arch/riscv/kernel/setup.c  | 234 +
>  arch/riscv/kernel/signal.c | 258 +++
>  arch/riscv/kernel/smp.c| 107 ++
>  arch/riscv/kernel/smpboot.c| 105 ++
>  arch/riscv/kernel/stacktrace.c | 183 
>  arch/riscv/kernel/sys_riscv.c  |  85 
>  arch/riscv/kernel/syscall_table.c  |  26 +++
>  arch/riscv/kernel/time.c   | 116 +++
>  arch/riscv/kernel/traps.c  | 167 +++
>  arch/riscv/kernel/vdso.c   | 125 +++
>  arch/riscv/kernel/vdso/.gitignore  |   1 +
>  arch/riscv/kernel/vdso/Makefile|  61 ++
>  arch/riscv/kernel/vdso/sigreturn.S |  25 +++
>  arch/riscv/kernel/vdso/vdso.S  |  28 +++
>  arch/riscv/kernel/vdso/vdso.lds.S  |  77 +++
>  arch/riscv/kernel/vmlinux.lds.S|  93 +
>  31 files changed, 3714 insertions(+)
>  create mode 100644 arch/riscv/kernel/Makefile
>  create mode 100644 arch/riscv/kernel/asm-offsets.c
>  create mode 100644 arch/riscv/kernel/cacheinfo.c
>  create mode 100644 arch/riscv/kernel/cpu.c
>  create mode 100644 arch/riscv/kernel/entry.S
>  create mode 100644 arch/riscv/kernel/head.S
>  create mode 100644 arch/riscv/kernel/irq.c
>  create mode 100644 arch/riscv/kernel/module.c
>  create mode 100644 arch/riscv/kernel/pci.c
>  create mode 100644 arch/riscv/kernel/plic.c
>  create mode 100644 arch/riscv/kernel/process.c
>  create mode 100644 arch/riscv/kernel/ptrace.c
>  create mode 100644 arch/riscv/kernel/reset.c
>  create mode 100644 arch/riscv/kernel/riscv_ksyms.c
>  create mode 100644 arch/riscv/kernel/sbi-con.c
>  create mode 100644 arch/riscv/kernel/setup.c
>  create mode 100644 arch/riscv/kernel/signal.c
>  create mode 100644 arch/riscv/kernel/smp.c
>  create mode 100644 arch/riscv/kernel/smpboot.c
>  create mode 100644 arch/riscv/kernel/stacktrace.c
>  create mode 100644 arch/riscv/kernel/sys_riscv.c
>  create mode 100644 arch/riscv/kernel/syscall_table.c
>  create mode 100644 arch/riscv/kernel/time.c
>  create mode 100644 arch/riscv/kernel/traps.c
>  create mode 100644 arch/riscv/kernel/vdso.c
>  create mode 100644 arch/riscv/kernel/vdso/.gitignore
>  create mode 100644 arch/riscv/kernel/vdso/Makefile
>  create mode 100644 arch/riscv/kernel/vdso/sigreturn.S
>  create mode 100644 arch/riscv/kernel/vdso/vdso.S
>  create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S
>  create mode 100644 arch/riscv/kernel/vmlinux.lds.S

What's missing from this patchset (ideally) is a good writeup under
DOcumentation/ on expectations of system state (and/or configuration)
upon entry of the kernel. For comparison, see the arm64 documentation
where they were quite specific in this.


This patch is also pushing size limits, and is getting unwieldy to
comment on. I'll point out a few things below with plenty of snipped
out lines.

>
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> new file mode 100644
> index ..94ac2931c56a
> --- /dev/null
> +++ b/arch/riscv/kernel/Makefile
> @@ -0,0 +1,19 @@
> +#
> +# Makefile for the RISC-V Linux kernel
> +#
> +
> +extra-y := head.o vmlinux.lds
> +
> +obj-y  := cpu.o entry.o irq.o process.o ptrace.o reset.o setup.o \
> +  signal.o syscall_table.o sys_riscv.o time.o traps.o \
> +  riscv_ksyms.o stacktrace.o vdso.o cacheinfo.o vdso/
> +
> +CFLAGS_setup.o := -mcmodel=medany
> +
> +obj-$(CONFIG_SMP)  += smpboot.o smp.o
> +obj-$(CONFIG_SBI_CONSOLE)  += sbi-con.o
> +obj-$(CONFIG_PCI)  += pci.o
> +obj-$(CONFIG_MODULES)  += module.o
> +obj-$(CONFIG_PLIC) += plic.o
> +
> +clean:
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> new file mode 100644
> index ..ac2e0cfaf8a3
> --- /dev/null
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -0,0 +1,113 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the

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

2017-05-22 Thread Olof Johansson 
On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt  wrote:
> ---
>  arch/riscv/kernel/Makefile |  19 ++
>  arch/riscv/kernel/asm-offsets.c| 113 ++
>  arch/riscv/kernel/cacheinfo.c  |  82 
>  arch/riscv/kernel/cpu.c|  81 
>  arch/riscv/kernel/entry.S  | 414 
> +
>  arch/riscv/kernel/head.S   | 139 +
>  arch/riscv/kernel/irq.c| 205 ++
>  arch/riscv/kernel/module.c | 185 +
>  arch/riscv/kernel/pci.c|  36 
>  arch/riscv/kernel/plic.c   | 208 +++
>  arch/riscv/kernel/process.c| 130 
>  arch/riscv/kernel/ptrace.c | 148 +
>  arch/riscv/kernel/reset.c  |  33 +++
>  arch/riscv/kernel/riscv_ksyms.c|  16 ++
>  arch/riscv/kernel/sbi-con.c| 214 +++
>  arch/riscv/kernel/setup.c  | 234 +
>  arch/riscv/kernel/signal.c | 258 +++
>  arch/riscv/kernel/smp.c| 107 ++
>  arch/riscv/kernel/smpboot.c| 105 ++
>  arch/riscv/kernel/stacktrace.c | 183 
>  arch/riscv/kernel/sys_riscv.c  |  85 
>  arch/riscv/kernel/syscall_table.c  |  26 +++
>  arch/riscv/kernel/time.c   | 116 +++
>  arch/riscv/kernel/traps.c  | 167 +++
>  arch/riscv/kernel/vdso.c   | 125 +++
>  arch/riscv/kernel/vdso/.gitignore  |   1 +
>  arch/riscv/kernel/vdso/Makefile|  61 ++
>  arch/riscv/kernel/vdso/sigreturn.S |  25 +++
>  arch/riscv/kernel/vdso/vdso.S  |  28 +++
>  arch/riscv/kernel/vdso/vdso.lds.S  |  77 +++
>  arch/riscv/kernel/vmlinux.lds.S|  93 +
>  31 files changed, 3714 insertions(+)
>  create mode 100644 arch/riscv/kernel/Makefile
>  create mode 100644 arch/riscv/kernel/asm-offsets.c
>  create mode 100644 arch/riscv/kernel/cacheinfo.c
>  create mode 100644 arch/riscv/kernel/cpu.c
>  create mode 100644 arch/riscv/kernel/entry.S
>  create mode 100644 arch/riscv/kernel/head.S
>  create mode 100644 arch/riscv/kernel/irq.c
>  create mode 100644 arch/riscv/kernel/module.c
>  create mode 100644 arch/riscv/kernel/pci.c
>  create mode 100644 arch/riscv/kernel/plic.c
>  create mode 100644 arch/riscv/kernel/process.c
>  create mode 100644 arch/riscv/kernel/ptrace.c
>  create mode 100644 arch/riscv/kernel/reset.c
>  create mode 100644 arch/riscv/kernel/riscv_ksyms.c
>  create mode 100644 arch/riscv/kernel/sbi-con.c
>  create mode 100644 arch/riscv/kernel/setup.c
>  create mode 100644 arch/riscv/kernel/signal.c
>  create mode 100644 arch/riscv/kernel/smp.c
>  create mode 100644 arch/riscv/kernel/smpboot.c
>  create mode 100644 arch/riscv/kernel/stacktrace.c
>  create mode 100644 arch/riscv/kernel/sys_riscv.c
>  create mode 100644 arch/riscv/kernel/syscall_table.c
>  create mode 100644 arch/riscv/kernel/time.c
>  create mode 100644 arch/riscv/kernel/traps.c
>  create mode 100644 arch/riscv/kernel/vdso.c
>  create mode 100644 arch/riscv/kernel/vdso/.gitignore
>  create mode 100644 arch/riscv/kernel/vdso/Makefile
>  create mode 100644 arch/riscv/kernel/vdso/sigreturn.S
>  create mode 100644 arch/riscv/kernel/vdso/vdso.S
>  create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S
>  create mode 100644 arch/riscv/kernel/vmlinux.lds.S

What's missing from this patchset (ideally) is a good writeup under
DOcumentation/ on expectations of system state (and/or configuration)
upon entry of the kernel. For comparison, see the arm64 documentation
where they were quite specific in this.


This patch is also pushing size limits, and is getting unwieldy to
comment on. I'll point out a few things below with plenty of snipped
out lines.

>
> diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> new file mode 100644
> index ..94ac2931c56a
> --- /dev/null
> +++ b/arch/riscv/kernel/Makefile
> @@ -0,0 +1,19 @@
> +#
> +# Makefile for the RISC-V Linux kernel
> +#
> +
> +extra-y := head.o vmlinux.lds
> +
> +obj-y  := cpu.o entry.o irq.o process.o ptrace.o reset.o setup.o \
> +  signal.o syscall_table.o sys_riscv.o time.o traps.o \
> +  riscv_ksyms.o stacktrace.o vdso.o cacheinfo.o vdso/
> +
> +CFLAGS_setup.o := -mcmodel=medany
> +
> +obj-$(CONFIG_SMP)  += smpboot.o smp.o
> +obj-$(CONFIG_SBI_CONSOLE)  += sbi-con.o
> +obj-$(CONFIG_PCI)  += pci.o
> +obj-$(CONFIG_MODULES)  += module.o
> +obj-$(CONFIG_PLIC) += plic.o
> +
> +clean:
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> new file mode 100644
> index ..ac2e0cfaf8a3
> --- /dev/null
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -0,0 +1,113 @@
> +/*
> + * 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