Re: [PATCH 01/52] [microblaze] Kconfig patches

[Michal Simek <monstr@monstr.eu>]

Hi Randy,

thanks for review. Comments are below.
>> diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
>> new file mode 100644
>> index 0000000..6dbc069
>> --- /dev/null
>> +++ b/arch/microblaze/Kconfig
>> @@ -0,0 +1,160 @@
>> +# For a description of the syntax of this configuration file,
>> +# see Documentation/kbuild/config-language.txt.
>>     
>
> It's                          kconfig-language.txt.
>   
I fix it
>> +
>> +mainmenu "Linux/Microblaze Kernel Configuration"
>> +
>> +config MICROBLAZE
>> +	bool
>> +	default y
>>     
>
> The one-line form:
> 	def_bool y
> is preferred, just to be more concise, use less screen real estate...
> Above and many times below.
>   
fix.

>> +config MMU
>> +	bool
>> +	default n
>> +
>> +config SWAP
>> +	bool
>> +	default n
>> +
>> +config RWSEM_GENERIC_SPINLOCK
>> +	bool
>> +	default y
>> +
>> +config RWSEM_XCHGADD_ALGORITHM
>> +	bool
>> +
>> +
>> +config ARCH_HAS_ILOG2_U32
>> +	bool
>> +	default n
>> +
>> +config ARCH_HAS_ILOG2_U64
>> +	bool
>> +	default n
>> +
>> +config GENERIC_FIND_NEXT_BIT
>> +	bool
>> +	default y
>> +
>> +config GENERIC_HWEIGHT
>> +	bool
>> +	default y
>> +
>> +config GENERIC_HARDIRQS
>> +	bool
>> +	default y
>> +
>> +config GENERIC_IRQ_PROBE
>> +	bool
>> +	default y
>> +
>> +config GENERIC_CALIBRATE_DELAY
>> +	bool
>> +	default y
>> +
>> +config PCI
>> +	bool
>> +	default n
>> +
>> +config UID16
>> +	bool
>> +	default y
>> +
>> +config DEFCONFIG_LIST
>> +	string
>> +	default "arch/$ARCH/defconfig"
>> +
>> +source "init/Kconfig"
>> +
>> +source "arch/microblaze/platform/Kconfig.platform"
>> +
>> +menu "Processor type and features"
>> +config PREEMPT
>> +	bool "Preemptible Kernel"
>> +	help
>> +	  This option reduces the latency of the kernel when reacting to
>> +	  real-time or interactive events by allowing a low priority process to
>> +	  be preempted even if it is in kernel mode executing a system call.
>> +	  This allows applications to run more reliably even when the system is
>> +	  under load.
>> +
>> +	  Say Y here if you are building a kernel for a desktop, embedded
>> +	  or real-time system.  Say N if you are unsure.
>> +
>> +config PREEMPT_TIMES
>> +	bool "Collect preemption latency times"
>> +	depends on PREEMPT
>> +	help
>> +	  Allow collection for preemption latency times.
>> +
>> +config XILINX_UNCACHED_SHADOW
>> +    bool "Are you using uncached shadow for RAM ?"
>> +    depends on MICROBLAZE
>> +    default y
>> +    help
>> +      This is needed to be able to allocate uncachable memory regions.
>>     
>
> We don't seem to spell uncacheable consistently.
>
> Kernel source tree:
> 	uncachable - 27 times
> 	uncacheable - 53 times
> google:
> 	uncachable - 11,900 hits
> 	uncacheable - 26,700 hits
>
> so I would use "uncacheable".
>
>   
I don't know if is spelling correct. But I tried to find count of uncached
in kernel tree and a result is interesting. For me is not problem to 
change it.

$ grep -rn "uncached" * | grep -v "microblaze" | grep "*" | wc -l
198
 
>> +      The feature requires the design to define the RAM memory controller window
>> +      to be twice as large as the actual physical memory.
>> +
>> +config LARGE_ALLOCS
>> +       bool "Allow allocating large blocks (> 1MB) of memory"
>> +       help
>> +            Allow the slab memory allocator to keep chains for very large
>> +            memory sizes - upto 32MB. You may need this if your system has
>>     
>
> s/upto/up to/
>   
fix.
>> +            a lot of RAM, and you need to able to allocate very large
>> +            contiguous chunks. If unsure, say N.
>>     
>
> Please indent config symbol help text as indicated in
> Documentation/CodingStyle:  one tab + 2 spaces for each line of help text.
> (Please check all help text.)
>   
fix in all Kconfig files.
>> +comment "Boot options"
>> +
>> +config CMDLINE
>> +	string "Default kernel command string"
>> +	default ""
>> +	help
>> +	  On some architectures there is currently no way for the boot loader
>> +	  to pass arguments to the kernel. For these architectures, you should
>> +	  supply some command-line options at build time by entering them 
>> +	  here. 
>> +
>> +config CMDLINE_FORCE
>> +	bool "Force default kernel command string"
>> +	help
>> +	  Set this to have arguments from the default kernel command string 
>> +	  override those passed by the boot loader
>>     
>
> End with period (".").
>   
fix in all Kconfig files.
>> +
>> +config OF
>> +	bool
>> +	default y
>> +	help
>> +	  Set this to turn on OF
>>     
>
> End with period.  Is this Open Firmware?
>
>   
>> +config OF_DEVICE
>> +	bool
>> +	default y
>> +	help
>> +	  Set this to turn on OF
>>     
>
> Ditto.
>   
Yes. Microblaze architecture use Open Firmware files. I set to this 
value with def_boot y without help part.
>> diff --git a/arch/microblaze/Kconfig.debug b/arch/microblaze/Kconfig.debug
>> new file mode 100644
>> index 0000000..1ccdd84
>> --- /dev/null
>> +++ b/arch/microblaze/Kconfig.debug
>> @@ -0,0 +1,22 @@
>> +menu "Kernel hacking"
>> +
>> +source "lib/Kconfig.debug"
>> +
>> +config EARLY_PRINTK
>> +	bool "EARLY_PRINTK"
>> +	default y
>> +
>> +config EARLY_PRINTK_UARTLITE_ADDRESS
>> +	hex "Physical address where UART Lite for early printk is mapped"
>> +	depends on EARLY_PRINTK
>> +	default "0x40600000"
>> +	help
>> +	  Please enter physcal address where your uart lite is mapped
>>     
>
> End with period.  s/physcal/physical/
>   
fix + all period faults.
>> +endchoice
>> +
>> +# This is stil a bit broken - disabling for now JW 20070504
>>     
>
> s/stil/still/
>   
fix
>> diff --git a/arch/microblaze/platform/generic/Kconfig.auto b/arch/microblaze/platform/generic/Kconfig.auto
>> new file mode 100644
>> index 0000000..751369b
>> --- /dev/null
>> +++ b/arch/microblaze/platform/generic/Kconfig.auto
>> @@ -0,0 +1,45 @@
>> +#
>> +# Platform Kconfig menu for Microblaze generic board
>> +#
>> +
>> +config KERNEL_BASE_ADDR
>> +	hex "Physical address where Linux Kernel is"
>> +	default "0x44000000"
>> +	help
>> +	  BASE Address for kernel
>> +
>> +config XILINX_ERAM_SIZE
>> +       hex "Size address of XILINX_RAM" if ALLOW_EDIT_AUTO && XILINX_UNCACHED_SHADOW
>>     
>
> What is a "Size address"?
>   
Size address is physical size of main memory - fix.
>> +       default 0x02000000
>> +
>> +comment "Definitions for MICROBLAZE0"
>> +      depends on ALLOW_EDIT_AUTO
>> +
>> +config XILINX_MICROBLAZE0_FAMILY
>> +       string "Targetted FPGA family" if ALLOW_EDIT_AUTO
>> +       default spartan3e
>> +
>> +config XILINX_MICROBLAZE0_HW_VER
>> +       string "Core version number" if ALLOW_EDIT_AUTO
>> +       default 5.00.c
>> +
>> +config XILINX_MICROBLAZE0_USE_MSR_INSTR
>> +       int "USE_MSR_INSTR" if ALLOW_EDIT_AUTO
>> +       default 1
>> +
>> +config XILINX_MICROBLAZE0_USE_BARREL
>> +       int "USE_BARREL range (0:1)" if ALLOW_EDIT_AUTO
>> +       default 1
>>     
>
> These (above & below) look like booleans.  If you really need a
> range, config symbols can be specified as ranges.  See
> Documentation/kbuild/kconfig-language.txt.
>   
Fix is with range property.
>> +config XILINX_MICROBLAZE0_USE_DIV
>> +       int "USE_DIV range (0:1)" if ALLOW_EDIT_AUTO
>> +       default 1
>> +
>> +config XILINX_MICROBLAZE0_USE_HW_MUL
>> +       int "USE_HW_MUL range (0:1)" if ALLOW_EDIT_AUTO
>> +       default 1
>> +
>> +config XILINX_MICROBLAZE0_USE_FPU
>> +       int "USE_FPU range (0:1)" if ALLOW_EDIT_AUTO
>> +       default 0
>> +
>> -- 
>>     
>
> ---
> ~Randy
Regards,
Michal Simek