From: Sudeep Holla <sudeep.holla@arm.com> To: Mark Rutland <mark.rutland@arm.com> Cc: Sudeep Holla <sudeep.holla@arm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>, Catalin Marinas <Catalin.Marinas@arm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Will Deacon <Will.Deacon@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 7/9] ARM64: kernel: add support for cpu cache information Date: Fri, 27 Jun 2014 12:22:17 +0100 [thread overview] Message-ID: <53AD53E9.8030105@arm.com> (raw) In-Reply-To: <20140627103611.GE7262@leverpostej> Hi Mark, Thanks for the review. On 27/06/14 11:36, Mark Rutland wrote: > Hi Sudeep, > > On Wed, Jun 25, 2014 at 06:30:42PM +0100, Sudeep Holla wrote: >> From: Sudeep Holla <sudeep.holla@arm.com> >> >> This patch adds support for cacheinfo on ARM64. >> >> On ARMv8, the cache hierarchy can be identified through Cache Level ID >> (CLIDR) register while the cache geometry is provided by Cache Size ID >> (CCSIDR) register. >> >> Since the architecture doesn't provide any way of detecting the cpus >> sharing particular cache, device tree is used for the same purpose. >> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> --- >> arch/arm64/kernel/Makefile | 3 +- >> arch/arm64/kernel/cacheinfo.c | 135 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 137 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/kernel/cacheinfo.c > > [...] > >> +static inline enum cache_type get_cache_type(int level) >> +{ >> + unsigned int clidr; >> + >> + if (level > MAX_CACHE_LEVEL) >> + return CACHE_TYPE_NOCACHE; >> + asm volatile ("mrs %0, clidr_el1" : "=r" (clidr)); > > Can't that allocate a w register? > That should be fine, as all of these cache info registers are 32-bit. > You can make clidr a u64 to avoid that. > What would be the preference ? Using w registers for all these cache registers or using u64 with x registers? >> + return CLIDR_CTYPE(clidr, level); >> +} >> + >> +/* >> + * NumSets, bits[27:13] - (Number of sets in cache) - 1 >> + * Associativity, bits[12:3] - (Associativity of cache) - 1 >> + * LineSize, bits[2:0] - (Log2(Number of words in cache line)) - 2 >> + */ >> +#define CCSIDR_WRITE_THROUGH BIT(31) >> +#define CCSIDR_WRITE_BACK BIT(30) >> +#define CCSIDR_READ_ALLOCATE BIT(29) >> +#define CCSIDR_WRITE_ALLOCATE BIT(28) >> +#define CCSIDR_LINESIZE_MASK 0x7 >> +#define CCSIDR_ASSOCIAT_SHIFT 3 >> +#define CCSIDR_ASSOCIAT_MASK 0x3FF > > ASSOCIAT doesn't quite roll off of the tongue... > I have no idea why I chose that incomplete name :( >> +#define CCSIDR_NUMSETS_SHIFT 13 >> +#define CCSIDR_NUMSETS_MASK 0x7FF >> + >> +/* >> + * Which cache CCSIDR represents depends on CSSELR value >> + * Make sure no one else changes CSSELR during this >> + * smp_call_function_single prevents preemption for us >> + */ >> +static inline u32 get_ccsidr(u32 csselr) >> +{ >> + u32 ccsidr; >> + >> + /* Put value into CSSELR */ >> + asm volatile("msr csselr_el1, %x0" : : "r" (csselr)); > > This looks a little dodgy. I think GCC can leave the upper 32 bits in a > random state. Why not cast csselr to a u64 here? > >> + isb(); >> + /* Read result out of CCSIDR */ >> + asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr)); >> + >> + return ccsidr; > > Similarly it might make sense to make the temporary variable a u64. > > [...] > >> +int init_cache_level(unsigned int cpu) >> +{ >> + unsigned int ctype, level = 1, leaves = 0; >> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); >> + >> + if (!this_cpu_ci) >> + return -EINVAL; >> + >> + do { >> + ctype = get_cache_type(level); >> + if (ctype == CACHE_TYPE_NOCACHE) >> + break; >> + /* Separate instruction and data caches */ >> + leaves += (ctype == CACHE_TYPE_SEPARATE) ? 2 : 1; >> + } while (++level <= MAX_CACHE_LEVEL); > > I think this would be clearer with: > > for (level = 1; level <= MAX_CACHE_LEVEL; level++) > > We do something like that in populate_cache_leaves below. > Right will change it.
WARNING: multiple messages have this Message-ID (diff)
From: sudeep.holla@arm.com (Sudeep Holla) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 7/9] ARM64: kernel: add support for cpu cache information Date: Fri, 27 Jun 2014 12:22:17 +0100 [thread overview] Message-ID: <53AD53E9.8030105@arm.com> (raw) In-Reply-To: <20140627103611.GE7262@leverpostej> Hi Mark, Thanks for the review. On 27/06/14 11:36, Mark Rutland wrote: > Hi Sudeep, > > On Wed, Jun 25, 2014 at 06:30:42PM +0100, Sudeep Holla wrote: >> From: Sudeep Holla <sudeep.holla@arm.com> >> >> This patch adds support for cacheinfo on ARM64. >> >> On ARMv8, the cache hierarchy can be identified through Cache Level ID >> (CLIDR) register while the cache geometry is provided by Cache Size ID >> (CCSIDR) register. >> >> Since the architecture doesn't provide any way of detecting the cpus >> sharing particular cache, device tree is used for the same purpose. >> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Cc: linux-arm-kernel at lists.infradead.org >> --- >> arch/arm64/kernel/Makefile | 3 +- >> arch/arm64/kernel/cacheinfo.c | 135 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 137 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/kernel/cacheinfo.c > > [...] > >> +static inline enum cache_type get_cache_type(int level) >> +{ >> + unsigned int clidr; >> + >> + if (level > MAX_CACHE_LEVEL) >> + return CACHE_TYPE_NOCACHE; >> + asm volatile ("mrs %0, clidr_el1" : "=r" (clidr)); > > Can't that allocate a w register? > That should be fine, as all of these cache info registers are 32-bit. > You can make clidr a u64 to avoid that. > What would be the preference ? Using w registers for all these cache registers or using u64 with x registers? >> + return CLIDR_CTYPE(clidr, level); >> +} >> + >> +/* >> + * NumSets, bits[27:13] - (Number of sets in cache) - 1 >> + * Associativity, bits[12:3] - (Associativity of cache) - 1 >> + * LineSize, bits[2:0] - (Log2(Number of words in cache line)) - 2 >> + */ >> +#define CCSIDR_WRITE_THROUGH BIT(31) >> +#define CCSIDR_WRITE_BACK BIT(30) >> +#define CCSIDR_READ_ALLOCATE BIT(29) >> +#define CCSIDR_WRITE_ALLOCATE BIT(28) >> +#define CCSIDR_LINESIZE_MASK 0x7 >> +#define CCSIDR_ASSOCIAT_SHIFT 3 >> +#define CCSIDR_ASSOCIAT_MASK 0x3FF > > ASSOCIAT doesn't quite roll off of the tongue... > I have no idea why I chose that incomplete name :( >> +#define CCSIDR_NUMSETS_SHIFT 13 >> +#define CCSIDR_NUMSETS_MASK 0x7FF >> + >> +/* >> + * Which cache CCSIDR represents depends on CSSELR value >> + * Make sure no one else changes CSSELR during this >> + * smp_call_function_single prevents preemption for us >> + */ >> +static inline u32 get_ccsidr(u32 csselr) >> +{ >> + u32 ccsidr; >> + >> + /* Put value into CSSELR */ >> + asm volatile("msr csselr_el1, %x0" : : "r" (csselr)); > > This looks a little dodgy. I think GCC can leave the upper 32 bits in a > random state. Why not cast csselr to a u64 here? > >> + isb(); >> + /* Read result out of CCSIDR */ >> + asm volatile("mrs %0, ccsidr_el1" : "=r" (ccsidr)); >> + >> + return ccsidr; > > Similarly it might make sense to make the temporary variable a u64. > > [...] > >> +int init_cache_level(unsigned int cpu) >> +{ >> + unsigned int ctype, level = 1, leaves = 0; >> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); >> + >> + if (!this_cpu_ci) >> + return -EINVAL; >> + >> + do { >> + ctype = get_cache_type(level); >> + if (ctype == CACHE_TYPE_NOCACHE) >> + break; >> + /* Separate instruction and data caches */ >> + leaves += (ctype == CACHE_TYPE_SEPARATE) ? 2 : 1; >> + } while (++level <= MAX_CACHE_LEVEL); > > I think this would be clearer with: > > for (level = 1; level <= MAX_CACHE_LEVEL; level++) > > We do something like that in populate_cache_leaves below. > Right will change it.
next prev parent reply other threads:[~2014-06-27 11:21 UTC|newest] Thread overview: 130+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-06-25 17:30 [PATCH 0/9] drivers: cacheinfo support Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` [PATCH 1/9] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla 2014-06-25 17:30 ` [PATCH 2/9] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 22:23 ` Russell King - ARM Linux 2014-06-25 22:23 ` Russell King - ARM Linux 2014-06-25 22:23 ` Russell King - ARM Linux 2014-06-25 22:23 ` Russell King - ARM Linux 2014-06-26 18:41 ` Sudeep Holla 2014-06-26 18:41 ` Sudeep Holla 2014-06-26 18:41 ` Sudeep Holla 2014-06-26 18:41 ` Sudeep Holla 2014-06-26 18:41 ` Sudeep Holla 2014-06-26 18:50 ` Russell King - ARM Linux 2014-06-26 18:50 ` Russell King - ARM Linux 2014-06-26 18:50 ` Russell King - ARM Linux 2014-06-26 18:50 ` Russell King - ARM Linux 2014-06-26 18:50 ` Russell King - ARM Linux 2014-06-26 19:03 ` Sudeep Holla 2014-06-26 19:03 ` Sudeep Holla 2014-06-26 19:03 ` Sudeep Holla 2014-06-26 19:03 ` Sudeep Holla 2014-06-26 19:03 ` Sudeep Holla 2014-07-10 0:09 ` Greg Kroah-Hartman 2014-07-10 0:09 ` Greg Kroah-Hartman 2014-07-10 0:09 ` Greg Kroah-Hartman 2014-07-10 0:09 ` Greg Kroah-Hartman 2014-07-10 13:37 ` Sudeep Holla 2014-07-10 13:37 ` Sudeep Holla 2014-07-10 13:37 ` Sudeep Holla 2014-07-10 13:37 ` Sudeep Holla 2014-07-10 13:37 ` Sudeep Holla 2014-06-25 17:30 ` [PATCH 3/9] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` [PATCH 4/9] s390: " Sudeep Holla 2014-06-25 17:30 ` [PATCH 5/9] x86: " Sudeep Holla 2014-06-25 17:30 ` [PATCH 6/9] powerpc: " Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 17:30 ` [PATCH 7/9] ARM64: kernel: add support for cpu cache information Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-27 10:36 ` Mark Rutland 2014-06-27 10:36 ` Mark Rutland 2014-06-27 11:22 ` Sudeep Holla [this message] 2014-06-27 11:22 ` Sudeep Holla 2014-06-27 11:34 ` Mark Rutland 2014-06-27 11:34 ` Mark Rutland 2014-06-25 17:30 ` [PATCH 8/9] ARM: " Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 22:33 ` Russell King - ARM Linux 2014-06-25 22:33 ` Russell King - ARM Linux 2014-06-26 11:33 ` Sudeep Holla 2014-06-26 11:33 ` Sudeep Holla 2014-06-26 0:19 ` Stephen Boyd 2014-06-26 0:19 ` Stephen Boyd 2014-06-26 11:36 ` Sudeep Holla 2014-06-26 11:36 ` Sudeep Holla 2014-06-26 18:45 ` Stephen Boyd 2014-06-26 18:45 ` Stephen Boyd 2014-06-27 9:38 ` Sudeep Holla 2014-06-27 9:38 ` Sudeep Holla 2014-06-25 17:30 ` [PATCH 9/9] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla 2014-06-25 17:30 ` Sudeep Holla 2014-06-25 22:37 ` Russell King - ARM Linux 2014-06-25 22:37 ` Russell King - ARM Linux 2014-06-26 13:02 ` Sudeep Holla 2014-06-26 13:02 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 0/9] drivers: cacheinfo support Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 1/9] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla 2014-07-25 19:09 ` Stephen Boyd 2014-07-28 13:37 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 2/9] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla 2014-07-29 23:09 ` Stephen Boyd 2014-07-30 16:23 ` Sudeep Holla 2014-07-31 19:46 ` Stephen Boyd 2014-08-05 18:15 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 3/9] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 4/9] s390: " Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 5/9] x86: " Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 6/9] powerpc: " Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 7/9] ARM64: kernel: add support for cpu cache information Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 8/9] ARM: " Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-07-25 16:44 ` [PATCH v2 9/9] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla 2014-07-25 16:44 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 00/11] drivers: cacheinfo support Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 01/11] cpumask: factor out show_cpumap into separate helper function Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 02/11] topology: replace custom attribute macros with standard DEVICE_ATTR* Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 03/11] drivers: base: add new class "cpu" to group cpu devices Sudeep Holla 2014-08-21 11:20 ` David Herrmann 2014-08-21 12:30 ` Sudeep Holla 2014-08-21 12:37 ` David Herrmann 2014-08-21 14:54 ` Sudeep Holla 2014-08-22 9:12 ` Kay Sievers 2014-08-22 11:29 ` [PATCH] drivers: base: add cpu_device_create to support per-cpu devices Sudeep Holla 2014-08-22 11:37 ` David Herrmann 2014-08-22 11:41 ` David Herrmann 2014-08-22 12:33 ` Sudeep Holla 2014-08-26 16:54 ` Sudeep Holla 2014-08-26 17:08 ` David Herrmann 2014-08-22 12:17 ` Sudeep Holla 2014-09-02 17:22 ` Sudeep Holla 2014-09-02 17:26 ` Greg Kroah-Hartman 2014-09-02 17:40 ` Sudeep Holla 2014-09-02 17:55 ` Greg Kroah-Hartman 2014-08-21 10:59 ` [PATCH v3 04/11] drivers: base: support cpu cache information interface to userspace via sysfs Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 05/11] ia64: move cacheinfo sysfs to generic cacheinfo infrastructure Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 06/11] s390: " Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 07/11] x86: " Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 08/11] powerpc: " Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 09/11] ARM64: kernel: add support for cpu cache information Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 10/11] ARM: " Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla 2014-08-21 10:59 ` [PATCH v3 11/11] ARM: kernel: add outer cache support for cacheinfo implementation Sudeep Holla 2014-08-21 10:59 ` Sudeep Holla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=53AD53E9.8030105@arm.com \ --to=sudeep.holla@arm.com \ --cc=Catalin.Marinas@arm.com \ --cc=Lorenzo.Pieralisi@arm.com \ --cc=Will.Deacon@arm.com \ --cc=heiko.carstens@de.ibm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.