| Message ID | 20221207-arm64-sysreg-helpers-v1-4-149fa1308a23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/cpufeature: Make use of sysreg helpers for hwcaps | expand |
On Thu, Dec 08, 2022 at 04:03:25PM +0000, Mark Brown wrote: > ID_AA64PFR0_EL1.FP and ID_AA64PFR0_EL1.AdvSIMD are both signed enumerations, > specify them as such in sysreg. There are other signed enumerations in the > registers but these are the only ones for which we currently use FTR_SIGNED, > others can be fixed up incrementally. Can we please do that in one go (either in one patch or a set of patches in this series)? I appreciate that's more work up-front, but doing that will mean that all the definitions are in a consistent state, which'll be far less error prone going forwards -- people will *definitely* forget to change the other existing definitions to be SIGNED if all that is hidden at the point-of-use. If we do that, we may as well explicitly annotate the UNSIGNED enums (and those which are purely enums without a sign) at the same time. That'll indicate that we've reviewed each entry, and it'll make it far more obvious one must do so when adding new entries in future. Thanks, Mark. > > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > arch/arm64/tools/sysreg | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg > index e8fb6684d7f3..16488c72827b 100644 > --- a/arch/arm64/tools/sysreg > +++ b/arch/arm64/tools/sysreg > @@ -846,12 +846,12 @@ Enum 27:24 GIC > 0b0001 IMP > 0b0010 V4P1 > EndEnum > -Enum 23:20 AdvSIMD > +SignedEnum 23:20 AdvSIMD > 0b0000 IMP > 0b0001 FP16 > 0b1111 NI > EndEnum > -Enum 19:16 FP > +SignedEnum 19:16 FP > 0b0000 IMP > 0b0001 FP16 > 0b1111 NI > > -- > 2.30.2 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Dec 09, 2022 at 12:42:10PM +0000, Mark Rutland wrote: > On Thu, Dec 08, 2022 at 04:03:25PM +0000, Mark Brown wrote: > > ID_AA64PFR0_EL1.FP and ID_AA64PFR0_EL1.AdvSIMD are both signed enumerations, > > specify them as such in sysreg. There are other signed enumerations in the > > registers but these are the only ones for which we currently use FTR_SIGNED, > > others can be fixed up incrementally. > Can we please do that in one go (either in one patch or a set of patches in > this series)? > I appreciate that's more work up-front, but doing that will mean that all the > definitions are in a consistent state, which'll be far less error prone going > forwards -- people will *definitely* forget to change the other existing > definitions to be SIGNED if all that is hidden at the point-of-use. I am not sure I will get round to doing all that at once in a reasonable timeframe on what is basically a low priority background task. I'd much rather just leave the use of FTR_SIGNED/UNSIGNED in the C code, I only did this because I initially did that conversion and the repetitiveness was jumping out as obvious. > If we do that, we may as well explicitly annotate the UNSIGNED enums (and those > which are purely enums without a sign) at the same time. That'll indicate that > we've reviewed each entry, and it'll make it far more obvious one must do so > when adding new entries in future. We could also just leave Enum as unspecified, do all the UnsignedEnums, and leave enum as unspecified and not generating a sign constant, that any users that care about the sign of an enum won't get an incorrect default.
On Fri, Dec 09, 2022 at 01:33:49PM +0000, Mark Brown wrote: > On Fri, Dec 09, 2022 at 12:42:10PM +0000, Mark Rutland wrote: > > On Thu, Dec 08, 2022 at 04:03:25PM +0000, Mark Brown wrote: > > > > ID_AA64PFR0_EL1.FP and ID_AA64PFR0_EL1.AdvSIMD are both signed enumerations, > > > specify them as such in sysreg. There are other signed enumerations in the > > > registers but these are the only ones for which we currently use FTR_SIGNED, > > > others can be fixed up incrementally. > > > Can we please do that in one go (either in one patch or a set of patches in > > this series)? > > > I appreciate that's more work up-front, but doing that will mean that all the > > definitions are in a consistent state, which'll be far less error prone going > > forwards -- people will *definitely* forget to change the other existing > > definitions to be SIGNED if all that is hidden at the point-of-use. > > I am not sure I will get round to doing all that at once in a reasonable > timeframe on what is basically a low priority background task. I'd much > rather just leave the use of FTR_SIGNED/UNSIGNED in the C code, I only > did this because I initially did that conversion and the repetitiveness > was jumping out as obvious. > > > If we do that, we may as well explicitly annotate the UNSIGNED enums (and those > > which are purely enums without a sign) at the same time. That'll indicate that > > we've reviewed each entry, and it'll make it far more obvious one must do so > > when adding new entries in future. > > We could also just leave Enum as unspecified, do all the UnsignedEnums, > and leave enum as unspecified and not generating a sign constant, that > any users that care about the sign of an enum won't get an incorrect > default. Sure, that works for me! Thanks, Mark.
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg index e8fb6684d7f3..16488c72827b 100644 --- a/arch/arm64/tools/sysreg +++ b/arch/arm64/tools/sysreg @@ -846,12 +846,12 @@ Enum 27:24 GIC 0b0001 IMP 0b0010 V4P1 EndEnum -Enum 23:20 AdvSIMD +SignedEnum 23:20 AdvSIMD 0b0000 IMP 0b0001 FP16 0b1111 NI EndEnum -Enum 19:16 FP +SignedEnum 19:16 FP 0b0000 IMP 0b0001 FP16 0b1111 NI
ID_AA64PFR0_EL1.FP and ID_AA64PFR0_EL1.AdvSIMD are both signed enumerations, specify them as such in sysreg. There are other signed enumerations in the registers but these are the only ones for which we currently use FTR_SIGNED, others can be fixed up incrementally. Signed-off-by: Mark Brown <broonie@kernel.org> --- arch/arm64/tools/sysreg | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)