| Message ID | 20241107025831.5459-1-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] armv8: Fix get_sctlr() return type | expand |
On Thu, 7 Nov 2024 at 04:58, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > SCTLR_EL2 is a 64-bit register [1]. Return its value as long (64 bit) > instead of int (32 bit) in get_sctlr() to make sure it's not trimmed. > > [1] https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en > > Fixes: 0ae7653128c8 ("arm64: core support") > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > Changes in v2: > - None (this patch was introduced in v2) > > arch/arm/cpu/armv8/cache_v8.c | 2 +- > arch/arm/include/asm/system.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > index e6be6359c5d9..5d6953ffedd1 100644 > --- a/arch/arm/cpu/armv8/cache_v8.c > +++ b/arch/arm/cpu/armv8/cache_v8.c > @@ -825,7 +825,7 @@ void dcache_enable(void) > > void dcache_disable(void) > { > - uint32_t sctlr; > + unsigned long sctlr; Although that's correct since it's a 64bit platform, isn't it better to define it as u64 to be sure we'll have at least 64 bits? Thanks /Ilias > > sctlr = get_sctlr(); > > diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h > index 52f6c9b934d7..dbf9ab43e280 100644 > --- a/arch/arm/include/asm/system.h > +++ b/arch/arm/include/asm/system.h > @@ -171,7 +171,7 @@ static inline unsigned int current_el(void) > return 3 & (el >> 2); > } > > -static inline unsigned int get_sctlr(void) > +static inline unsigned long get_sctlr(void) > { > unsigned int el; > unsigned long val; > -- > 2.39.5 >
On Mon, Nov 11, 2024 at 8:48 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Thu, 7 Nov 2024 at 04:58, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > SCTLR_EL2 is a 64-bit register [1]. Return its value as long (64 bit) > > instead of int (32 bit) in get_sctlr() to make sure it's not trimmed. > > > > [1] https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en > > > > Fixes: 0ae7653128c8 ("arm64: core support") > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Changes in v2: > > - None (this patch was introduced in v2) > > > > arch/arm/cpu/armv8/cache_v8.c | 2 +- > > arch/arm/include/asm/system.h | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > index e6be6359c5d9..5d6953ffedd1 100644 > > --- a/arch/arm/cpu/armv8/cache_v8.c > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > @@ -825,7 +825,7 @@ void dcache_enable(void) > > > > void dcache_disable(void) > > { > > - uint32_t sctlr; > > + unsigned long sctlr; > > Although that's correct since it's a 64bit platform, isn't it better > to define it as u64 to be sure we'll have at least 64 bits? > Thanks for reviewing this! I considered making it uint64_t while reworking this bit. This 'sctlr' variable is assigned to get_sctlr() return value, and then passed to set_sctlr() as a parameter, both of which are 'unsigned long'. So I made 'sctlr' unsigned long too, just to match the signatures of mentioned functions. As for get_sctlr(), I just followed the style already existed inside and around that function. As you said, both u64 and unsigned long are 64-bit long types on ARMv8, and that's definitely ARMv8 specific file, so I guess that should be fine? In any case, if you have a strong opinion about this, please let me know and I'll rework it; both u64 and unsigned long look ok to me in this context. > Thanks > /Ilias > > > > sctlr = get_sctlr(); > > > > diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h > > index 52f6c9b934d7..dbf9ab43e280 100644 > > --- a/arch/arm/include/asm/system.h > > +++ b/arch/arm/include/asm/system.h > > @@ -171,7 +171,7 @@ static inline unsigned int current_el(void) > > return 3 & (el >> 2); > > } > > > > -static inline unsigned int get_sctlr(void) > > +static inline unsigned long get_sctlr(void) > > { > > unsigned int el; > > unsigned long val; > > -- > > 2.39.5 > >
On Wed, 13 Nov 2024 at 04:30, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Mon, Nov 11, 2024 at 8:48 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Thu, 7 Nov 2024 at 04:58, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > > > > > SCTLR_EL2 is a 64-bit register [1]. Return its value as long (64 bit) > > > instead of int (32 bit) in get_sctlr() to make sure it's not trimmed. > > > > > > [1] https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en > > > > > > Fixes: 0ae7653128c8 ("arm64: core support") > > > Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > > --- > > > Changes in v2: > > > - None (this patch was introduced in v2) > > > > > > arch/arm/cpu/armv8/cache_v8.c | 2 +- > > > arch/arm/include/asm/system.h | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c > > > index e6be6359c5d9..5d6953ffedd1 100644 > > > --- a/arch/arm/cpu/armv8/cache_v8.c > > > +++ b/arch/arm/cpu/armv8/cache_v8.c > > > @@ -825,7 +825,7 @@ void dcache_enable(void) > > > > > > void dcache_disable(void) > > > { > > > - uint32_t sctlr; > > > + unsigned long sctlr; > > > > Although that's correct since it's a 64bit platform, isn't it better > > to define it as u64 to be sure we'll have at least 64 bits? > > > > Thanks for reviewing this! I considered making it uint64_t while > reworking this bit. This 'sctlr' variable is assigned to get_sctlr() > return value, and then passed to set_sctlr() as a parameter, both of > which are 'unsigned long'. So I made 'sctlr' unsigned long too, just > to match the signatures of mentioned functions. As for get_sctlr(), I > just followed the style already existed inside and around that > function. As you said, both u64 and unsigned long are 64-bit long > types on ARMv8, and that's definitely ARMv8 specific file, so I guess > that should be fine? In any case, if you have a strong opinion about > this, please let me know and I'll rework it; both u64 and unsigned > long look ok to me in this context. Yea perhaps I am nitpicking too much Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > Thanks > > /Ilias > > > > > > sctlr = get_sctlr(); > > > > > > diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h > > > index 52f6c9b934d7..dbf9ab43e280 100644 > > > --- a/arch/arm/include/asm/system.h > > > +++ b/arch/arm/include/asm/system.h > > > @@ -171,7 +171,7 @@ static inline unsigned int current_el(void) > > > return 3 & (el >> 2); > > > } > > > > > > -static inline unsigned int get_sctlr(void) > > > +static inline unsigned long get_sctlr(void) > > > { > > > unsigned int el; > > > unsigned long val; > > > -- > > > 2.39.5 > > >
On Wed, 06 Nov 2024 20:58:30 -0600, Sam Protsenko wrote: > SCTLR_EL2 is a 64-bit register [1]. Return its value as long (64 bit) > instead of int (32 bit) in get_sctlr() to make sure it's not trimmed. > > [1] https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en > > Applied to u-boot/next, thanks!
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c index e6be6359c5d9..5d6953ffedd1 100644 --- a/arch/arm/cpu/armv8/cache_v8.c +++ b/arch/arm/cpu/armv8/cache_v8.c @@ -825,7 +825,7 @@ void dcache_enable(void) void dcache_disable(void) { - uint32_t sctlr; + unsigned long sctlr; sctlr = get_sctlr(); diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 52f6c9b934d7..dbf9ab43e280 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -171,7 +171,7 @@ static inline unsigned int current_el(void) return 3 & (el >> 2); } -static inline unsigned int get_sctlr(void) +static inline unsigned long get_sctlr(void) { unsigned int el; unsigned long val;
SCTLR_EL2 is a 64-bit register [1]. Return its value as long (64 bit) instead of int (32 bit) in get_sctlr() to make sure it's not trimmed. [1] https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL2--System-Control-Register--EL2-?lang=en Fixes: 0ae7653128c8 ("arm64: core support") Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- Changes in v2: - None (this patch was introduced in v2) arch/arm/cpu/armv8/cache_v8.c | 2 +- arch/arm/include/asm/system.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)