From owner-freebsd-arm@FreeBSD.ORG Thu Nov 22 16:48:28 2012 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 7EC451CF for ; Thu, 22 Nov 2012 16:48:28 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from mail-ie0-f182.google.com (mail-ie0-f182.google.com [209.85.223.182]) by mx1.freebsd.org (Postfix) with ESMTP id 330B98FC14 for ; Thu, 22 Nov 2012 16:48:28 +0000 (UTC) Received: by mail-ie0-f182.google.com with SMTP id s9so7287847iec.13 for ; Thu, 22 Nov 2012 08:48:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:subject:mime-version:content-type:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to:x-mailer :x-gm-message-state; bh=PrRfeKVgDu0LJ8DQzRscp5bP9x0RK4kxleksJdxNO/k=; b=VecZTpCCXFGtOWC3RiD09dysOmaX7oFEEvFu/GtghHmHZiRR4VLJSsC4zCo0EtwL/t CJjPLWBfDR5pdFIwpgEH6ElwM8PBzrXcZg5+Zb+qSEAxn4zG9KwiymVpe5flUpCI1LbS P6A4wI2/2mhU47Vhi+EEqqHQMXS0/yizoFBFfp3+ejYTgPZAm7tVT6Y3M0xOhy6RHsG8 D9Lvu02yek7GX7Uy88L32pFRCoc4cOoFJYb0AV2FFAoGcm5Bm8/OKQOYmSIeFDrel28g qji576iZNfNQ6NcKV6ZyP2iTtF0t6QomFmgTYTivzOa+Iz/Jcg48xbxovYcMdtJM3Jm9 WvOQ== Received: by 10.50.5.239 with SMTP id v15mr1152403igv.41.1353602907410; Thu, 22 Nov 2012 08:48:27 -0800 (PST) Received: from 53.imp.bsdimp.com (50-78-194-198-static.hfc.comcastbusiness.net. [50.78.194.198]) by mx.google.com with ESMTPS id bh3sm2386190igc.0.2012.11.22.08.46.56 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 22 Nov 2012 08:48:25 -0800 (PST) Sender: Warner Losh Subject: Re: ARM/SMP, Some patches for review. Mime-Version: 1.0 (Apple Message framework v1085) Content-Type: text/plain; charset=utf-8 From: Warner Losh In-Reply-To: <50AE3C0F.20809@semihalf.com> Date: Thu, 22 Nov 2012 09:46:25 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: <94A3C1F1-0138-4D11-85BC-9E988AEF2E9F@bsdimp.com> References: <50AA4E87.3000505@semihalf.com> <50ACE2B4.8010904@semihalf.com> <50AE3C0F.20809@semihalf.com> To: =?utf-8?Q?=C5=81ukasz_P=C5=82achno?= X-Mailer: Apple Mail (2.1085) X-Gm-Message-State: ALoCoQlndsdEo3Q1qB3C0z32r5dzCz58VQESi2cazErb7rL2c/wvNEw9wPMEvZeUkuv71SfwfcyN Cc: freebsd-arm@freebsd.org, cognet@freebsd.org X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Porting FreeBSD to the StrongARM Processor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Nov 2012 16:48:28 -0000 On Nov 22, 2012, at 7:51 AM, =C5=81ukasz P=C5=82achno wrote: > On 21.11.2012 17:00, Giovanni Trematerra wrote: >> On Wed, Nov 21, 2012 at 3:18 PM, =C5=81ukasz P=C5=82achno = wrote: >>> On 20.11.2012 00:08, Giovanni Trematerra wrote: >>>>=20 >>>> On Mon, Nov 19, 2012 at 4:21 PM, =C5=81ukasz P=C5=82achno = wrote: >>>>>=20 >>>>> Hi, >>>>>=20 >>>>> I would like to propose few changes for ARM specific code. >>>>> Three attached patches for freebsd-current allows building = SMP-safe world >>>>> for ARM targets and turns on TEX remap for ARMv6 and ARMv7 = targets. >>>>>=20 >>>>> More details inside patch files. >>>>>=20 >>>>> 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. >>>>>=20 >>>>> I do not think adding hack like this: >>>>> #ifndef LOCORE >>>>> #define LOCORE >>>>> #endif >>>>>=20 >>>>> 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 >>>>=20 >>>>=20 >>>> In commit-2.txt >>>> you should include style changes in sys/arm/arm/cpufunc_asm_armv7.S >>>> into a different patch. >>>=20 >>>=20 >>> fixed >>>=20 >>>=20 >>>>=20 >>>> @@ -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 >>>>=20 >>>> 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. >>>>=20 >>>=20 >>> Fixed, updated set of patches is attached. >>>=20 >>> - TEX remap is supported only for armv6 (changed to avoid breaking = armv6 >>> targets) >>> - Fixed issues with build for pre-armv6 targets (tested with make = tinderbox >>> TARGETS=3Darm >>>=20 >>=20 >> 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. >>=20 >> 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 */ >>=20 >> 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 */ >>=20 >> - arm11_sleep, /* sleep */ >> + armv7_cpu_sleep, /* sleep */ >>=20 >> /* Soft functions */ >>=20 >> cpufunc_null_fixup, /* dataabt_fixup */ >> cpufunc_null_fixup, /* prefetchabt_fixup */ >>=20 >> - arm11_context_switch, /* context_switch */ >> + armv7_context_switch, /* context_switch */ >>=20 >> cortexa_setup /* cpu setup */ >> }; >>=20 >=20 > I agree, but with this change I included also: >=20 > diff --git a/sys/arm/arm/cpufunc.c b/sys/arm/arm/cpufunc.c > index dd43c27..1d6f93f 100644 > --- a/sys/arm/arm/cpufunc.c > +++ b/sys/arm/arm/cpufunc.c > @@ -1049,14 +1049,14 @@ struct cpu_functions cortexa_cpufuncs =3D { > =09 > armv7_tlb_flushID, /* tlb_flushID */ > armv7_tlb_flushID_SE, /* tlb_flushID_SE */ > - arm11_tlb_flushI, /* tlb_flushI */ > - arm11_tlb_flushI_SE, /* tlb_flushI_SE */ > - arm11_tlb_flushD, /* tlb_flushD */ > - arm11_tlb_flushD_SE, /* tlb_flushD_SE */ > + armv7_tlb_flushID, /* tlb_flushI */ > + armv7_tlb_flushID_SE, /* tlb_flushI_SE */ > + armv7_tlb_flushID, /* tlb_flushD */ > + armv7_tlb_flushID_SE, /* tlb_flushD_SE */ >=20 > Changes merged into patch 1_SMP_fixes.diff >=20 >>=20 >> 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. >>=20 >> I'm not sure changes in sys/arm/arm/locore.S are style ones. >=20 > None of changes in this patch are related to style. In this patch I = wanted to improve code readability, not remove style conflicts. >=20 >>=20 >> I think that things like this aren't so readable. >> #if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0 >>=20 >> 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? >>=20 >> I mean in sys/arm/arm/cpuconf.h we could make something like this >>=20 >> #if (ARM_ARCH_6 + ARM_ARCH_7A) !=3D 0 >> #define ARM_ARCH_6_7A >> #endif >=20 > Changed to ARM_ARCH_6_7A >=20 >>=20 >> 3_kernel_trampoline.diff >> I think we should not make kernel_trampoline at all for the = unsupported CPUs. >> I propose this change to Makefile.arm >>=20 >> 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 >>=20 >> .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 >>=20 >> +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 >>=20 >> MKMODULESENV+=3D MACHINE=3D${MACHINE} >>=20 >=20 > Change merged into commit 3. I really don't like this part of the change. The if is ok, but the grep = isn't. It should be in the kernel config file as an option. I can be = in std.XXX files easily. >> 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. >>=20 >=20 > #defines reordered >=20 >> I'll try to test the pachset ASAP. >>=20 >=20 > New set of patches attached. I'll take a look at these in more detail... Warner > Regards, > =C5=81ukasz P=C5=82achno >=20 > = <1_SMP_fixes.diff><2_ARM_cleanup.diff><3_trampoline.diff><4_tex_remap.diff= ><5_memory_barriers.diff>_______________________________________________ > freebsd-arm@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-arm > To unsubscribe, send any mail to "freebsd-arm-unsubscribe@freebsd.org"