Date: Wed, 21 Nov 2012 17:00:01 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: =?UTF-8?B?xYF1a2FzeiBQxYJhY2hubw==?= <luk@semihalf.com> Cc: freebsd-arm@freebsd.org, cognet@freebsd.org Subject: Re: ARM/SMP, Some patches for review. Message-ID: <CACfq091X6BK7S5jenr46=iwpsyGEz1FdnRLMhXhPdZ4jd06g0w@mail.gmail.com> In-Reply-To: <50ACE2B4.8010904@semihalf.com> References: <50AA4E87.3000505@semihalf.com> <CACfq093LakTKEf8E3L5S7QnYcgdnBVpnFSRJo%2BMx=mfgbXpg0g@mail.gmail.com> <50ACE2B4.8010904@semihalf.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Nov 21, 2012 at 3:18 PM, =C5=81ukasz P=C5=82achno <luk@semihalf.com=
> wrote:
> On 20.11.2012 00:08, Giovanni Trematerra wrote:
>>
>> On Mon, Nov 19, 2012 at 4:21 PM, =C5=81ukasz P=C5=82achno <luk@semihalf.=
com> wrote:
>>>
>>> Hi,
>>>
>>> I would like to propose few changes for ARM specific code.
>>> Three attached patches for freebsd-current allows building SMP-safe wor=
ld
>>> for ARM targets and turns on TEX remap for ARMv6 and ARMv7 targets.
>>>
>>> More details inside patch files.
>>>
>>> Change introduced by "commit-2" removes armv7 targets (armv7 and pj4b)
>>> from
>>> kernel.tramp.
>>> AFAIK this feature is not working properly for armv7 targets and is
>>> causing
>>> problem during compilation:
>>> - LOCORE is defined during kernel compilation but not defined during
>>> kernel.tramp compilation, so #include pmap.h causes build errors.
>>>
>>> I do not think adding hack like this:
>>> #ifndef LOCORE
>>> #define LOCORE
>>> #endif
>>>
>>> to allow building something that is already broken is a good idea, so I
>>> removed cpufunc_asm_pj4b.S and cpufunc_asm_armv7.S from Makefile.arm
>>
>>
>> In commit-2.txt
>> you should include style changes in sys/arm/arm/cpufunc_asm_armv7.S
>> into a different patch.
>
>
> fixed
>
>
>>
>> @@ -63,7 +64,6 @@ FILES_CPU_FUNC =3D $S/$M/$M/cpufunc_asm_arm7tdmi.=
S \
>> $S/$M/$M/cpufunc_asm_xscale.S $S/$M/$M/cpufunc_asm.S \
>> $S/$M/$M/cpufunc_asm_xscale_c3.S $S/$M/$M/cpufunc_asm_armv5_ec.=
S
>> \
>> $S/$M/$M/cpufunc_asm_fa526.S $S/$M/$M/cpufunc_asm_sheeva.S \
>> - $S/$M/$M/cpufunc_asm_pj4b.S $S/$M/$M/cpufunc_asm_armv7.S
>>
>> You left a trailing back slash but beside that you should clean up
>> sys/arm/arm/elf_trampoline.c
>> and not make kernel.tramp to build at all for armv7 cpus or you'll end
>> up with a linker error
>> during generation of the kernel.tramp.
>>
>
> Fixed, updated set of patches is attached.
>
> - TEX remap is supported only for armv6 (changed to avoid breaking armv6
> targets)
> - Fixed issues with build for pre-armv6 targets (tested with make tinder=
box
> TARGETS=3Darm
>
1_SMP_fixes.diff
You'll endup to get a panic for PandaBoard systems.
The arm11 functions don't handle the SMP case.
So I propose to merge the changes below or commit them first.
Index: sys/arm/arm/cpufunc.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- sys/arm/arm/cpufunc.c (revision 243182)
+++ sys/arm/arm/cpufunc.c (working copy)
@@ -1079,18 +1079,18 @@ struct cpu_functions cortexa_cpufuncs =3D {
/* Other functions */
cpufunc_nullop, /* flush_prefetchbuf */
- arm11_drain_writebuf, /* drain_writebuf */
+ armv7_drain_writebuf, /* drain_writebuf */
cpufunc_nullop, /* flush_brnchtgt_C */
(void *)cpufunc_nullop, /* flush_brnchtgt_E */
- arm11_sleep, /* sleep */
+ armv7_cpu_sleep, /* sleep */
/* Soft functions */
cpufunc_null_fixup, /* dataabt_fixup */
cpufunc_null_fixup, /* prefetchabt_fixup */
- arm11_context_switch, /* context_switch */
+ armv7_context_switch, /* context_switch */
cortexa_setup /* cpu setup */
};
2_ARM_cleanup.diff
Changes to sys/arm/arm/machdep.c don't seem style changes and
they should live in a separate patch with a different motivation.
I'm not sure changes in sys/arm/arm/locore.S are style ones.
I think that things like this aren't so readable.
#if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0
Instead of things like that wouldn't be better to define different
macros when the sum is zero or non zero and stick with the
#if defined/!defined thing?
I mean in sys/arm/arm/cpuconf.h we could make something like this
#if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0
#define ARM_ARCH_6_7A
#endif
3_kernel_trampoline.diff
I think we should not make kernel_trampoline at all for the unsupported CPU=
s.
I propose this change to Makefile.arm
Index: sys/conf/Makefile.arm
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- sys/conf/Makefile.arm (revision 243182)
+++ sys/conf/Makefile.arm (working copy)
@@ -51,6 +51,7 @@ SYSTEM_LD_TAIL +=3D;sed s/" + SIZEOF_HEADERS"// ldsc
${SYSTEM_LD_}; \
${OBJCOPY} -S -O binary ${FULLKERNEL}.noheader \
${KERNEL_KO}.bin; \
+ ${NM} ${FULLKERNEL}.noheader | sort > ${FULLKERNEL}.map; \
rm ${FULLKERNEL}.noheader
.if defined(MFS_IMAGE)
@@ -62,9 +63,11 @@ FILES_CPU_FUNC =3D $S/$M/$M/cpufunc_asm_arm7tdmi.S \
$S/$M/$M/cpufunc_asm_sa1.S $S/$M/$M/cpufunc_asm_arm10.S \
$S/$M/$M/cpufunc_asm_xscale.S $S/$M/$M/cpufunc_asm.S \
$S/$M/$M/cpufunc_asm_xscale_c3.S $S/$M/$M/cpufunc_asm_armv5_ec.S \
$S/$M/$M/cpufunc_asm_fa526.S $S/$M/$M/cpufunc_asm_sheeva.S \
- $S/$M/$M/cpufunc_asm_pj4b.S $S/$M/$M/cpufunc_asm_armv7.S
+ $S/$M/$M/cpufunc_asm_armv6.S
+NO_TRAMP!=3D grep 'CPU_CORTEXA\|CPU_MV_PJ4B' opt_global.h || true ; echo
+
+.if ${NO_TRAMP} =3D=3D ""
KERNEL_EXTRA=3Dtrampoline
KERNEL_EXTRA_INSTALL=3Dkernel.gz.tramp
trampoline: ${KERNEL_KO}.tramp
@@ -110,6 +113,7 @@ ${KERNEL_KO}.tramp: ${KERNEL_KO} $S/$M/$M/inckern.
${KERNEL_KO}.gz.tramp.bin
rm ${KERNEL_KO}.tmp.gz ${KERNEL_KO}.tramp.noheader opt_kernname.h \
inflate-tramp.o tmphack.S
+.endif
MKMODULESENV+=3D MACHINE=3D${MACHINE}
4_tex-remap.diff
Some style(9) consideration.
#include(s) should be grouped together in alphabetical order.
So you should fix the pmap.h includes that you made.
I'll try to test the pachset ASAP.
--
Gianni
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq091X6BK7S5jenr46=iwpsyGEz1FdnRLMhXhPdZ4jd06g0w>