From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752831AbbJOKpc (ORCPT ); Thu, 15 Oct 2015 06:45:32 -0400 Received: from eu-smtp-delivery-143.mimecast.com ([146.101.78.143]:12540 "EHLO eu-smtp-delivery-143.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751563AbbJOKpa convert rfc822-to-8bit (ORCPT ); Thu, 15 Oct 2015 06:45:30 -0400 Subject: Re: [PATCH v3 09/24] arm64: Keep track of CPU feature registers To: Catalin Marinas References: <1444756952-31145-1-git-send-email-suzuki.poulose@arm.com> <1444756952-31145-10-git-send-email-suzuki.poulose@arm.com> <20151015103611.GK4239@e104818-lin.cambridge.arm.com> Cc: linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com, Vladimir.Murzin@arm.com, steve.capper@linaro.org, ryan.arnold@linaro.org, ard.biesheuvel@linaro.org, marc.zyngier@arm.com, will.deacon@arm.com, linux-kernel@vger.kernel.org, edward.nevill@linaro.org, aph@redhat.com, james.morse@arm.com, andre.przywara@arm.com, dave.martin@arm.com, christoffer.dall@linaro.org From: "Suzuki K. Poulose" Message-ID: <561F83C7.9090500@arm.com> Date: Thu, 15 Oct 2015 11:45:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20151015103611.GK4239@e104818-lin.cambridge.arm.com> X-OriginalArrivalTime: 15 Oct 2015 10:45:27.0626 (UTC) FILETIME=[9ABE82A0:01D10736] X-MC-Unique: gY9a9xc2RGCazXaK6fDRmQ-1 Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 15/10/15 11:36, Catalin Marinas wrote: > Hi Suzuki, > > Some minor comments below. >> The feature bits are classified as one of SCALAR_MIN, SCALAR_MAX and DISCRETE >> depending on the implication of the possible values. This information >> is used to decide the safe value for a feature. >> > > The "SCALAR..." etc. in the comment needs updating. Sorry about that. Will fix it. > >> +#define FTR_STRICT true >> +#define FTR_NONSTRICT false > > Please add a comment on what STRICT/NONSTRICT mean. > Sure >> +static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp) >> +{ >> + return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift); >> +} >> + >> +static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val) >> +{ >> + return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width); >> +} > > Slightly inconsistent naming: since you are prefixing everything with > arm64_, do the same for ftr_mask. OK, will add it. > >> +static struct arm64_ftr_reg arm64_ftr_regs[] = { >> + >> + /* This array should be always kept in the ascending order of id */ > > Do we have a sanity check somewhere? You could also call sort() on this > array and we wouldn't have to worry. No we don't have a sanity check. I will take a look. > >> +/* >> + * get_arm64_sys_reg - Lookup a feature register entry using its >> + * sys_reg() encoding. With the array arm64_ftr_regs sorted in the >> + * ascending order, we use binary search to find a matching entry. >> + * >> + * returns - Upon success, matching ftr_reg entry for id. >> + * - NULL on failure. It is upto the caller to decide >> + * the impact of a failure. >> + */ >> +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id) > > Nitpick: the * near the function name. > > Also rename it to get_arm64_ftr_reg() to match the actual return type. > Sure, will fix all of these. Thanks for the detailed look