On 21/10/16 14:59, James Greenhalgh wrote: > On Sat, Oct 15, 2016 at 07:38:40PM -0700, Andrew Pinski wrote: >> On Wed, Nov 25, 2015 at 11:59 AM, Andrew Pinski <pins...@gmail.com> wrote: >> Here is finally an updated (fixed) patch (I did not implement the two >> implementer big.LITTLE support yet, that will be for a different patch >> since I also fixed the part no not being unique as a separate patch. >> Once I get a new enough kernel, I will also look into doing the >> /sys/cpu/* style detection first. >> >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions >> (and tested hacking the location of the read in file to see if it >> works with big.LITTLE and other formats of /proc/cpuinfo). > > I'm OK with this in principle, but it needs some polish for pedantic > style comments... > >> * config/aarch64/aarch64-cores.def: Rewrite so IMP and PART are >> integer constants. >> * config/aarch64/driver-aarch64.c (struct aarch64_core_data): Change >> implementer_id to unsigned char. >> Change part_no to unsigned int. >> (AARCH64_BIG_LITTLE): New define. >> (INVALID_IMP): New define. >> (INVALID_CORE): New define. >> (cpu_data): Change the last element's implementer_id and part_no to integers. >> (valid_bL_string_p): Rewrite to .. >> (valid_bL_core_p): this for integers instead of strings. >> (parse_field): New function. >> (contains_string_p): Rewrite to ... >> (contains_core_p): this for integers and only for the part_no. >> (host_detect_local_cpu): Rewrite handling of implementation and part >> num to be integers; >> simplifying the code. > >> Index: config/aarch64/aarch64-cores.def >> =================================================================== >> --- config/aarch64/aarch64-cores.def (revision 241200) >> +++ config/aarch64/aarch64-cores.def (working copy) >> @@ -32,43 +32,46 @@ >> FLAGS are the bitwise-or of the traits that apply to that core. >> This need not include flags implied by the architecture. >> COSTS is the name of the rtx_costs routine to use. >> - IMP is the implementer ID of the CPU vendor. On a GNU/Linux system it >> can >> - be found in /proc/cpuinfo. >> + IMP is the implementer ID of the CPU vendor. On a GNU/Linux system it >> + can be found in /proc/cpuinfo. A partial list of implementer IDs is >> + given in the ARM Architecture Reference Manual ARMv8, for >> - in /proc/cpuinfo. For big.LITTLE systems this should have the form at of >> - "<big core part number>.<LITTLE core part number>". */ >> + in /proc/cpuinfo. For big.LITTLE systems this should use the macro >> AARCH64_BIG_LITTLE >> + where the big part number comes as the first arugment to the macro and >> little is the >> + second. */ > > Needs rewrapped for 80 char width. >
I don't think it's a good idea to line wrap the def files, some of them are processed with AWK during configure and having a complete entry per line avoids potential matching problems. R. >> >> -static bool >> -valid_bL_string_p (const char** core, const char* bL_string) >> + static bool >> +valid_bL_core_p (unsigned int *core, unsigned int bL_core) > > Stray space before static. > >> { >> - return strstr (bL_string, core[0]) != NULL >> - && strstr (bL_string, core[1]) != NULL; >> + return AARCH64_BIG_LITTLE (core[0], core[1]) == bL_core >> + || AARCH64_BIG_LITTLE (core[1], core[0]) == bL_core; >> +} >> + >> +/* Returns the integer that is after ':' for the field. */ >> +static unsigned parse_field (const char *field) > > parse_field should be on a new line, FIELD should be capitalised in the > explanatory comment. > > OK with the appropriate changes to rectify these points. > > Thanks, > James >