lkml.org 
[lkml]   [2011]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 33/36] Hexagon: Add configuration and makefiles for the Hexagon architecture.
From
Date
(I'd like to add some quick comments, Kconfig related. It's too late, I
guess.)

On Mon, 2011-10-31 at 18:55 -0500, Richard Kuo wrote:
> diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig
> new file mode 100644
> index 0000000..02513c2
> --- /dev/null
> +++ b/arch/hexagon/Kconfig
> @@ -0,0 +1,220 @@
> +# Hexagon configuration
> +comment "Linux Kernel Configuration for Hexagon"
> +
> +config HEXAGON
> + def_bool y
> + select HAVE_OPROFILE
> + select USE_GENERIC_SMP_HELPERS if SMP
> + # Other pending projects/to-do items.
> + # select HAVE_REGS_AND_STACK_ACCESS_API
> + # select HAVE_HW_BREAKPOINT if PERF_EVENTS
> + # select ARCH_HAS_CPU_IDLE_WAIT
> + # select ARCH_WANT_OPTIONAL_GPIOLIB
> + # select ARCH_REQUIRE_GPIOLIB
> + # select HAVE_CLK
> + # select IRQ_PER_CPU
> + select HAVE_IRQ_WORK
> + # select GENERIC_PENDING_IRQ if SMP

Is GENERIC_PENDING_IRQ also a pending project?

> + select GENERIC_ATOMIC64
> + select HAVE_PERF_EVENTS
> + select HAVE_GENERIC_HARDIRQS
> + select GENERIC_HARDIRQS_NO__DO_IRQ
> + select GENERIC_HARDIRQS_NO_DEPRECATED
> + # GENERIC_ALLOCATOR is used by dma_alloc_coherent()
> + select GENERIC_ALLOCATOR
> + select GENERIC_IRQ_SHOW
> + select HAVE_ARCH_KGDB
> + select HAVE_ARCH_TRACEHOOK
> + select NO_IOPORT
> + # mostly generic routines, with some accelerated ones

What does this comment on?

> + ---help---
> + Qualcomm Hexagon is a processor architecture designed for high
> + performance and low power across a wide variety of applications.
> +
> +config HEXAGON_ARCH_V1
> + bool
> +
> +config HEXAGON_ARCH_V2
> + bool
> +
> +config HEXAGON_ARCH_V3
> + bool
> +
> +config HEXAGON_ARCH_V4
> + bool

[...]

> +#config ZONE_DMA
> +# bool
> +# default y

Why is this added commented out?

> +config HAS_DMA
> + bool
> + select HAVE_DMA_ATTRS
> + default y

HAS_DMA isn't supposed to be used this way, is it? See commit
411f0f3edc141a582190d3605cadd1d993abb6df ("Introduce CONFIG_HAS_DMA").
Can't this entry be replaced by a "select HAVE_DMA_ATTRS" line in the
"config HEXAGON" entry? That seems to be the common idiom.

[...]

> +config STACKTRACE_SUPPORT
> + def_bool y
> + select STACKTRACE

Some grepping suggests that the tracing infrastructure will "select
STACKTRACE" if the architecture sets STACKTRACE_SUPPORT (tile apparently
also gets this wrong). Have I grepped this correctly?

> +config GENERIC_BUG
> + def_bool y
> + depends on BUG
> +
> +config BUG
> + def_bool y

Why do you have this? Other architecture don't (there's just one BUG
entry in all the Kconfig files).

> +menu "Machine selection"
> +
> +choice
> + prompt "System type"
> + default HEXAGON_ARCH_V2
> +
> +config HEXAGON_COMET
> + bool "Comet Board"
> + select HEXAGON_ARCH_V2
> + ---help---
> + Support for the Comet platform.
> +
> +endchoice

The default doesn't match the (single) config option here (it should
default to HEXAGON_COMET). That shouldn't matter because there's only
one option, which the config tools will then pick (that's the way they
seem to work).

But why is this a choice when there's nothing to actually choose?
Moreover, just looking at this patch suggests HEXAGON_COMET is yet
unused (CONFIG_HEXAGON_COMET is commented out in the Makefile). So at
first glance this seems an elaborate way to select HEXAGON_ARCH_V2.

[...]

> +config SMP
> + bool "Multi-Processing support"
> + ---help---
> + Enables SMP support in the kernel. If unsure, say "Y"

Odd. Even x86 and powerpc (the only two architectures I looked at)
suggest to say "N" to those not knowing what to do here.

> +config NR_CPUS
> + int "Maximum number of CPUs" if SMP
> + range 2 6 if SMP
> + default "1" if !SMP
> + default "6" if SMP
> + ---help---
> + This allows you to specify the maximum number of CPUs which this
> + kernel will support. The maximum supported value is 6 and the
> + minimum value which makes sense is 2.
> +
> + This is purely to save memory - each supported CPU adds
> + approximately eight kilobytes to the kernel image.

This is good. Quite a number of architectures do net set this
(especially the default of "1" in the !SMP case).

[...]

> +config GENERIC_GPIO
> + bool "Generic GPIO support"
> + default n

I know next to nothing about GENERIC_GPIO but does it make sense for
Hexagon? If not, wouldn't a "def_bool n" do? (Perhaps you could even
drop this entry entirely.)

[...]

> diff --git a/arch/hexagon/Makefile b/arch/hexagon/Makefile
> new file mode 100644
> index 0000000..7ce9563
> --- /dev/null
> +++ b/arch/hexagon/Makefile

[...]

> +# arch/hexagon/platform/common/
> +#
> +#core-$(CONFIG_HEXAGON_COMET) += arch/hexagon/platform/comet/
> +#machine-$(CONFIG_HEXAGON_COMET) := comet


Paul Bolle



\
 
 \ /
  Last update: 2011-11-01 10:33    [W:0.067 / U:2.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site