Messages in this thread | | | Subject | Re: [PATCH v5 33/36] Hexagon: Add configuration and makefiles for the Hexagon architecture. | From | Paul Bolle <> | Date | Tue, 01 Nov 2011 10:30:46 +0100 |
| |
(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
| |