diff mbox series

[v2,1/2] armv8: Fix get_sctlr() return type

Message ID 20241107025831.5459-1-semen.protsenko@linaro.org
State New
Headers show
Series [v2,1/2] armv8: Fix get_sctlr() return type | expand

Commit Message

Sam Protsenko Nov. 7, 2024, 2:58 a.m. UTC
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(-)

Comments

Ilias Apalodimas Nov. 11, 2024, 2:48 p.m. UTC | #1
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
>
Sam Protsenko Nov. 13, 2024, 2:30 a.m. UTC | #2
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
> >
diff mbox series

Patch

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;