Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241675 > unrolled thread

Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding Catalin Marinas <catalin.marinas@arm.com> - 2015-10-07 18:40 +0200
    Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-07 19:10 +0200
      Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding Catalin Marinas <catalin.marinas@arm.com> - 2015-10-08 16:50 +0200
        Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-08 18:20 +0200

#1241675 — Re: [PATCH v2 06/22] arm64: sys_reg: Define System register encoding

On Mon, Oct 05, 2015 at 06:01:55PM +0100, Suzuki K. Poulose wrote:
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -22,11 +22,11 @@
>  
>  #include <asm/opcodes.h>
>  
> -#define SCTLR_EL1_CP15BEN	(0x1 << 5)
> -#define SCTLR_EL1_SED		(0x1 << 8)
> -
>  /*
> - * ARMv8 ARM reserves the following encoding for system registers:
> + * sys_reg: Defines the ARMv8 ARM encoding for the System register.
> + *
> + * ARMv8 ARM reserves the following encoding for system registers in the
> + * instructions accessing them.

Nitpick: the sentence should end with a colon.

>   * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
>   *  C5.2, version:ARM DDI 0487A.f)
>   *	[20-19] : Op0
> @@ -34,15 +34,40 @@
>   *	[15-12] : CRn
>   *	[11-8]  : CRm
>   *	[7-5]   : Op2
> + * Hence we use [ sys_reg() << 5 ] in the mrs/msr instructions.

Do we really need to have all the ids shifted right by 5? I can't see
where it helps. OTOH, it makes the code more complicated by having to
remember to shift the id left by 5.

> + *

Nitpick: no need for another line.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241691

On 07/10/15 17:36, Catalin Marinas wrote:
> On Mon, Oct 05, 2015 at 06:01:55PM +0100, Suzuki K. Poulose wrote:
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -22,11 +22,11 @@
>>
>>   #include <asm/opcodes.h>
>>
>> -#define SCTLR_EL1_CP15BEN	(0x1 << 5)
>> -#define SCTLR_EL1_SED		(0x1 << 8)
>> -
>>   /*
>> - * ARMv8 ARM reserves the following encoding for system registers:
>> + * sys_reg: Defines the ARMv8 ARM encoding for the System register.
>> + *
>> + * ARMv8 ARM reserves the following encoding for system registers in the
>> + * instructions accessing them.
>
> Nitpick: the sentence should end with a colon.

OK

>
>>    * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
>>    *  C5.2, version:ARM DDI 0487A.f)
>>    *	[20-19] : Op0
>> @@ -34,15 +34,40 @@
>>    *	[15-12] : CRn
>>    *	[11-8]  : CRm
>>    *	[7-5]   : Op2
>> + * Hence we use [ sys_reg() << 5 ] in the mrs/msr instructions.
>
> Do we really need to have all the ids shifted right by 5? I can't see
> where it helps. OTOH, it makes the code more complicated by having to
> remember to shift the id left by 5.

This is a cosmetic change, to reuse the sys_reg() definitions for both
mrs_s/msr_s macros and the CPU ID. The (existing)left shift is only needed
for the mrs_s/msr_s, so the existing users don't have to worry about the shift
unless they have hard-open-coded values for the register. On the plus
side it becomes slightly easier to use the same encoding for CPU id
tracking (and manual decoding). If you think this is superfluous, I could
change the CPU-id to use right shifted values from sys_reg.

>
>> + *
>
> Nitpick: no need for another line.
>

OK

Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242451

On Wed, Oct 07, 2015 at 06:03:34PM +0100, Suzuki K. Poulose wrote:
> On 07/10/15 17:36, Catalin Marinas wrote:
> >On Mon, Oct 05, 2015 at 06:01:55PM +0100, Suzuki K. Poulose wrote:
> >>   * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
> >>   *  C5.2, version:ARM DDI 0487A.f)
> >>   *	[20-19] : Op0
> >>@@ -34,15 +34,40 @@
> >>   *	[15-12] : CRn
> >>   *	[11-8]  : CRm
> >>   *	[7-5]   : Op2
> >>+ * Hence we use [ sys_reg() << 5 ] in the mrs/msr instructions.
> >
> >Do we really need to have all the ids shifted right by 5? I can't see
> >where it helps. OTOH, it makes the code more complicated by having to
> >remember to shift the id left by 5.
> 
> This is a cosmetic change, to reuse the sys_reg() definitions for both
> mrs_s/msr_s macros and the CPU ID. The (existing)left shift is only needed
> for the mrs_s/msr_s, so the existing users don't have to worry about the shift
> unless they have hard-open-coded values for the register. On the plus
> side it becomes slightly easier to use the same encoding for CPU id
> tracking (and manual decoding). If you think this is superfluous, I could
> change the CPU-id to use right shifted values from sys_reg.

You may be right but I still fail to see whether the shifted values are
useful to the CPUID code. Are you referring to the 'switch' statements?

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242584

On 08/10/15 15:43, Catalin Marinas wrote:
> On Wed, Oct 07, 2015 at 06:03:34PM +0100, Suzuki K. Poulose wrote:
>> On 07/10/15 17:36, Catalin Marinas wrote:
>>> On Mon, Oct 05, 2015 at 06:01:55PM +0100, Suzuki K. Poulose wrote:
>>>>    * (Ref: ARMv8 ARM, Section: "System instruction class encoding overview",
>>>>    *  C5.2, version:ARM DDI 0487A.f)
>>>>    *	[20-19] : Op0
>>>> @@ -34,15 +34,40 @@
>>>>    *	[15-12] : CRn
>>>>    *	[11-8]  : CRm
>>>>    *	[7-5]   : Op2
>>>> + * Hence we use [ sys_reg() << 5 ] in the mrs/msr instructions.
>>>
>>> Do we really need to have all the ids shifted right by 5? I can't see
>>> where it helps. OTOH, it makes the code more complicated by having to
>>> remember to shift the id left by 5.
>>
>> This is a cosmetic change, to reuse the sys_reg() definitions for both
>> mrs_s/msr_s macros and the CPU ID. The (existing)left shift is only needed
>> for the mrs_s/msr_s, so the existing users don't have to worry about the shift
>> unless they have hard-open-coded values for the register. On the plus
>> side it becomes slightly easier to use the same encoding for CPU id
>> tracking (and manual decoding). If you think this is superfluous, I could
>> change the CPU-id to use right shifted values from sys_reg.
>
> You may be right but I still fail to see whether the shifted values are
> useful to the CPUID code. Are you referring to the 'switch' statements?
>

Yes. Again we can adjust the macros to get the right fields for switch().
Its also about the meaning of sys_reg(). The name kind of implies the system
register encoding defined by ARM ARM. However the current version
defines the encoding as it appears in the mrs/msr instructions, which can
be confusing if we start using the same encoding for representing the sys_reg
elsewhere(in this case CPUID infrastructure). Again, I am not too particular
about this change.

Suzuki


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web