| Message ID | 20171109210311.25655-1-jeremy.linton@arm.com |
|---|---|
| Headers | show |
| Series | Support PPTT for ARM64 | expand |
Included in ACPICA version 20171110 > -----Original Message----- > From: Jeremy Linton [mailto:jeremy.linton@arm.com] > Sent: Thursday, November 9, 2017 1:03 PM > To: linux-acpi@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org; sudeep.holla@arm.com; > hanjun.guo@linaro.org; lorenzo.pieralisi@arm.com; rjw@rjwysocki.net; > will.deacon@arm.com; catalin.marinas@arm.com; > gregkh@linuxfoundation.org; viresh.kumar@linaro.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux- > pm@vger.kernel.org; jhugo@codeaurora.org; wangxiongfeng2@huawei.com; > Jonathan.Zhang@cavium.com; ahs3@redhat.com; > Jayachandran.Nair@cavium.com; austinwc@codeaurora.org; lenb@kernel.org; > Moore, Robert <robert.moore@intel.com>; Zheng, Lv <lv.zheng@intel.com>; > devel@acpica.org; Jeremy Linton <jeremy.linton@arm.com> > Subject: [PATCH v4 1/9] ACPICA: Add additional PPTT flags for cache > properties > > The PPTT table has a number of flags that can be set to describe whether > the cache is I/D/U and the allocation and write policies. Add these > flags. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > include/acpi/actbl1.h | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h index > 6b8714a428b6..71f874e2790d 100644 > --- a/include/acpi/actbl1.h > +++ b/include/acpi/actbl1.h > @@ -1346,6 +1346,20 @@ struct acpi_pptt_cache { > #define ACPI_PPTT_MASK_CACHE_TYPE (0x0C) /* Cache type */ > #define ACPI_PPTT_MASK_WRITE_POLICY (0x10) /* Write policy */ > > +/* Attributes describing cache */ > +#define ACPI_PPTT_CACHE_READ_ALLOCATE (0x0) /* Cache line is > allocated on read */ > +#define ACPI_PPTT_CACHE_WRITE_ALLOCATE (0x01) /* Cache line is > allocated on write */ > +#define ACPI_PPTT_CACHE_RW_ALLOCATE (0x02) /* Cache line is > allocated on read and write */ > +#define ACPI_PPTT_CACHE_RW_ALLOCATE_ALT (0x03) /* Alternate > representation of above */ > + > +#define ACPI_PPTT_CACHE_TYPE_DATA (0x0) /* Data cache */ > +#define ACPI_PPTT_CACHE_TYPE_INSTR (1<<2) /* Instruction cache */ > +#define ACPI_PPTT_CACHE_TYPE_UNIFIED (2<<2) /* Unified I & D cache > */ > +#define ACPI_PPTT_CACHE_TYPE_UNIFIED_ALT (3<<2) /* Alternate > representation of above */ > + > +#define ACPI_PPTT_CACHE_POLICY_WB (0x0) /* Cache is write back > */ > +#define ACPI_PPTT_CACHE_POLICY_WT (1<<4) /* Cache is write > through */ > + > /* 2: ID Structure */ > > struct acpi_pptt_id { > -- > 2.13.5 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 09, 2017 at 03:03:07PM -0600, Jeremy Linton wrote: > The /sys cache entries should support ACPI/PPTT generated cache > topology information. Lets detect ACPI systems and call > an arch specific cache_setup_acpi() routine to update the hardware > probed cache topology. > > For arm64, if ACPI is enabled, determine the max number of cache > levels and populate them using a PPTT table if one is available. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/kernel/cacheinfo.c | 23 ++++++++++++++++++----- > drivers/acpi/pptt.c | 1 + > drivers/base/cacheinfo.c | 17 +++++++++++------ > include/linux/cacheinfo.h | 11 +++++++++-- > 4 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c > index 380f2e2fbed5..2e2cf0d312ba 100644 > --- a/arch/arm64/kernel/cacheinfo.c > +++ b/arch/arm64/kernel/cacheinfo.c > @@ -17,6 +17,7 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <linux/acpi.h> > #include <linux/cacheinfo.h> > #include <linux/of.h> > > @@ -44,9 +45,17 @@ static void ci_leaf_init(struct cacheinfo *this_leaf, > this_leaf->type = type; > } > > +#ifndef CONFIG_ACPI > +int acpi_find_last_cache_level(unsigned int cpu) > +{ > + /*ACPI kernels should be built with PPTT support*/ > + return 0; > +} > +#endif > + Why can't this be in generic acpi header #ifdef CONFIG_ACPI_PPTT int acpi_find_last_cache_level(unsigned int cpu) #else static int acpi_find_last_cache_level(unsigned int cpu) { return 0; } #endif or something similar to of_find_last_cache_level. I don't see why this has to be ARM64 specific. > static int __init_cache_level(unsigned int cpu) > { > - unsigned int ctype, level, leaves, of_level; > + unsigned int ctype, level, leaves, fw_level; > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); > > for (level = 1, leaves = 0; level <= MAX_CACHE_LEVEL; level++) { > @@ -59,15 +68,19 @@ static int __init_cache_level(unsigned int cpu) > leaves += (ctype == CACHE_TYPE_SEPARATE) ? 2 : 1; > } > > - of_level = of_find_last_cache_level(cpu); > - if (level < of_level) { > + if (acpi_disabled) > + fw_level = of_find_last_cache_level(cpu); > + else > + fw_level = acpi_find_last_cache_level(cpu); > + > + if (level < fw_level) { > /* > * some external caches not specified in CLIDR_EL1 > * the information may be available in the device tree > * only unified external caches are considered here > */ > - leaves += (of_level - level); > - level = of_level; > + leaves += (fw_level - level); > + level = fw_level; > } > > this_cpu_ci->num_levels = level; Better to keep any arm64 related changes in a separate patch. > diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c > index 9c9b8b4660e0..aa259502c4eb 100644 > --- a/drivers/acpi/pptt.c > +++ b/drivers/acpi/pptt.c > @@ -324,6 +324,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf, > struct acpi_pptt_cache *found_cache, > struct acpi_pptt_processor *cpu_node) > { > + this_leaf->firmware_node = cpu_node; > if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) > this_leaf->size = found_cache->size; > if (found_cache->flags & ACPI_PPTT_LINE_SIZE_VALID) > diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c > index eb3af2739537..8eca279e50d1 100644 > --- a/drivers/base/cacheinfo.c > +++ b/drivers/base/cacheinfo.c > @@ -86,7 +86,7 @@ static int cache_setup_of_node(unsigned int cpu) > static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, > struct cacheinfo *sib_leaf) > { > - return sib_leaf->of_node == this_leaf->of_node; > + return sib_leaf->firmware_node == this_leaf->firmware_node; > } > I am thinking it's better to even separate the DT and ACPI PPTT related changes. First refactor DT code renaming or adding new cacheinfo elements. Then add ACPI PPTT related changes. It helps to keep changes small at a time and easy for bisection in case of any issues. > /* OF properties to query for a given cache type */ > @@ -215,6 +215,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf, > } > #endif > > +int __weak cache_setup_acpi(unsigned int cpu) > +{ > + return -ENOTSUPP; > +} > + > static int cache_shared_cpu_map_setup(unsigned int cpu) > { > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu); > @@ -225,11 +230,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu) > if (this_cpu_ci->cpu_map_populated) > return 0; > > - if (of_have_populated_dt()) > + if (!acpi_disabled) > + ret = cache_setup_acpi(cpu); > + else if (of_have_populated_dt()) > ret = cache_setup_of_node(cpu); > - else if (!acpi_disabled) > - /* No cache property/hierarchy support yet in ACPI */ > - ret = -ENOTSUPP; > + > if (ret) > return ret; > > @@ -286,7 +291,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu) > > static void cache_override_properties(unsigned int cpu) > { > - if (of_have_populated_dt()) > + if (acpi_disabled && of_have_populated_dt()) > return cache_of_override_properties(cpu); > } > > diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h > index 6a524bf6a06d..d1e9b8e01981 100644 > --- a/include/linux/cacheinfo.h > +++ b/include/linux/cacheinfo.h > @@ -36,6 +36,9 @@ enum cache_type { > * @of_node: if devicetree is used, this represents either the cpu node in > * case there's no explicit cache node or the cache node itself in the > * device tree > + * @firmware_node: Shared with of_node. When not using DT, this may contain > + * pointers to other firmware based values. Particularly ACPI/PPTT > + * unique values. > * @disable_sysfs: indicates whether this node is visible to the user via > * sysfs or not > * @priv: pointer to any private data structure specific to particular > @@ -64,8 +67,10 @@ struct cacheinfo { > #define CACHE_ALLOCATE_POLICY_MASK \ > (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) > #define CACHE_ID BIT(4) > - > - struct device_node *of_node; > + union { > + struct device_node *of_node; > + void *firmware_node; > + }; I would prefer struct device_node *of_node; changed to struct fwnode_handle *fwnode; You can then have struct pptt_fwnode { <.....> /*below fwnode allocated using acpi_alloc_fwnode_static */ struct fwnode_handle *fwnode; }; This gives a good starting point to abstract DT and ACPI. If not now, we can later implement fwnode.ops=pptt_cache_ops and then use get property for both DT and ACPI. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 09, 2017 at 03:03:05PM -0600, Jeremy Linton wrote: > Its helpful to be able to lookup the acpi_processor_id > associated with a logical cpu. Provide an arm64 > helper to do this. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/include/asm/acpi.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 59cca1d6ec54..408e7989d3a2 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -86,6 +86,10 @@ static inline bool acpi_has_cpu_in_madt(void) > } > > struct acpi_madt_generic_interrupt *acpi_cpu_get_madt_gicc(int cpu); > +static inline u32 get_acpi_id_for_cpu(unsigned int cpu) > +{ > + return acpi_cpu_get_madt_gicc(cpu)->uid; > +} If I followed the series correctly, this function is used in 2/9 already. So this needs to be moved down in the series to avoid build failure during bisection. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/20/2017 10:56 AM, Sudeep Holla wrote: (trimming) >> * case there's no explicit cache node or the cache node itself in the >> * device tree >> + * @firmware_node: Shared with of_node. When not using DT, this may contain >> + * pointers to other firmware based values. Particularly ACPI/PPTT >> + * unique values. >> * @disable_sysfs: indicates whether this node is visible to the user via >> * sysfs or not >> * @priv: pointer to any private data structure specific to particular >> @@ -64,8 +67,10 @@ struct cacheinfo { >> #define CACHE_ALLOCATE_POLICY_MASK \ >> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) >> #define CACHE_ID BIT(4) >> - >> - struct device_node *of_node; >> + union { >> + struct device_node *of_node; >> + void *firmware_node; >> + }; > > I would prefer > struct device_node *of_node; > changed to > struct fwnode_handle *fwnode; > > You can then have > struct pptt_fwnode { > <.....> > /*below fwnode allocated using acpi_alloc_fwnode_static */ > struct fwnode_handle *fwnode; > }; > > This gives a good starting point to abstract DT and ACPI. > > If not now, we can later implement fwnode.ops=pptt_cache_ops and then > use get property for both DT and ACPI. I'm obviously confused why this keeps coming up. On the surface it sounds like a good idea. But then, given that I've actually implemented a portion of it, what becomes clear is that the PPTT isn't a good match. Converting the OF routines to use the fwnode is fairly straightforward, but that doesn't help the ACPI situation other than to create a lot of misleading code (and the possibility of creating nonstandard DSDT entries). The fact that this hasn't been done for other tables MADT/SLIT/SRAT/etc makes me wonder why we should do it for the PPTT? Particularly, when one considers fwnode is more a DSDT<->DT abstraction and thus has a lot of API surface that simply doesn't make any sense given the PPTT binary tree structure. Given that most of the fwnode routines are translating string properties (for example fwnode_property_read_string()) it might be possible to build a translator of some form which takes DT style properties and attempts to map them to the ACPI PPTT tree. What this adds I can't fathom, beyond the fact that suddenly the fwnode interface is a partial/brittle implementation where a large subset of the fwnode_operations will tend to be degenerate cases. The result likely will be a poorly implemented translator which breaks or is meaningless over a large part of the fwnode API surface. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/11/17 18:02, Jeremy Linton wrote: > On 11/20/2017 10:56 AM, Sudeep Holla wrote: > > (trimming) > >>> * case there's no explicit cache node or the cache node >>> itself in the * device tree + * @firmware_node: Shared with >>> of_node. When not using DT, this may contain + * pointers to >>> other firmware based values. Particularly ACPI/PPTT + * unique >>> values. * @disable_sysfs: indicates whether this node is visible >>> to the user via * sysfs or not * @priv: pointer to any private >>> data structure specific to particular @@ -64,8 +67,10 @@ struct >>> cacheinfo { #define CACHE_ALLOCATE_POLICY_MASK \ >>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE) #define CACHE_ID >>> BIT(4) - - struct device_node *of_node; + union { + >>> struct device_node *of_node; + void *firmware_node; + >>> }; >> >> I would prefer struct device_node *of_node; changed to struct >> fwnode_handle *fwnode; >> >> You can then have struct pptt_fwnode { <.....> /*below fwnode >> allocated using acpi_alloc_fwnode_static */ struct fwnode_handle >> *fwnode; }; >> >> This gives a good starting point to abstract DT and ACPI. >> >> If not now, we can later implement fwnode.ops=pptt_cache_ops and >> then use get property for both DT and ACPI. > > > I'm obviously confused why this keeps coming up. On the surface it > sounds like a good idea. But then, given that I've actually > implemented a portion of it, what becomes clear is that the PPTT > isn't a good match. Fair enough. > Converting the OF routines to use the fwnode is fairly > straightforward, but that doesn't help the ACPI situation other than > to create a lot of misleading code (and the possibility of creating > nonstandard DSDT entries). The fact that this hasn't been done for > other tables MADT/SLIT/SRAT/etc makes me wonder why we should do it > for the PPTT? > IRQ/IORT does use it. If we don't want to use it fine. But the union doesn't make sense and breaks the flow many other subsystems follow. Hence I raised. Sorry, I hadn't followed the last revision/discussion on this, my bad. But I had this thought since the beginning, hence I brought this up. > Particularly, when one considers fwnode is more a DSDT<->DT > abstraction and thus has a lot of API surface that simply doesn't > make any sense given the PPTT binary tree structure. Given that most > of the fwnode routines are translating string properties (for > example fwnode_property_read_string()) it might be possible to build > a translator of some form which takes DT style properties and > attempts to map them to the ACPI PPTT tree. What this adds I can't > fathom, beyond the fact that suddenly the fwnode interface is a > partial/brittle implementation where a large subset of the > fwnode_operations will tend to be degenerate cases. The result likely > will be a poorly implemented translator which breaks or is > meaningless over a large part of the fwnode API surface. Sure, I just mentioned ops thing, but that's optional. I just didn't like the union which has of_node and void ptr instead of fwhandle. I am fine if many agree that it's bad idea to use fwhandle here. -- Regards, Sudeep -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ACPI 6.2 adds the Processor Properties Topology Table (PPTT), which is used to describe the processor and cache topology. Ideally it is used to extend/override information provided by the hardware, but right now ARM64 is entirely dependent on firmware provided tables. This patch parses the table for the cache topology and CPU topology. New in the v4 we rename the topology cluster_id to physical_id to match the kernel macro using it, and as its unlikley to actually map to a cluster on an ACPI system. When we enable ACPI/PPTT for arm64 we map the physical_id to the PPTT node flagged as the physical package by the firmware. This results in topologies that match what the remainder of the system expects. For example on juno: [root@mammon-juno-rh topology]# lstopo-no-graphics Package L#0 L2 L#0 (1024KB) L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0) L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1) L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2) L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3) L2 L#1 (2048KB) L1d L#4 (32KB) + L1i L#4 (48KB) + Core L#4 + PU L#4 (P#4) L1d L#5 (32KB) + L1i L#5 (48KB) + Core L#5 + PU L#5 (P#5) HostBridge L#0 PCIBridge PCIBridge PCIBridge PCI 1095:3132 Block(Disk) L#0 "sda" PCIBridge PCI 1002:68f9 GPU L#1 "renderD128" GPU L#2 "card0" GPU L#3 "controlD64" PCIBridge PCI 11ab:4380 Net L#4 "enp8s0" Git tree at: http://linux-arm.org/git?p=linux-jlinton.git branch: pptt_v4 v3->v4: Suppress the "Found duplicate cache level/type..." message if the duplicate cache entry is actually a duplicate node. This allows cases like L1I and L1D nodes that point at the same L2 node to be accepted without the warning. Remove cluster/physical split code. Add a patch to rename cluster_id so that its clear the topology may not be referring to a cluster. Add additional ACPICA patch for the PPTT cache properties. This matches an outstanding ACPICA pull that should be merged in the near future. Replace a number of (struct*)((u8*)ptr+offset) constructs with ACPI_ADD_PTR Split out the topology parsing into an additional patch. Tweak the cpu topology code to terminate on either a level, or a flag. Add an additional function which retrives the physical package id for a given cpu. Various other comments/tweaks. v2->v3: Remove valid bit check on leaf nodes. Now simply being a leaf node is sufficient to verify the processor id against the ACPI processor ids (gotten from MADT). Use the acpi processor for the "level 0" Id. This makes the /sys visible core/thread ids more human readable if the firmware uses small consecutive values for processor ids. Added PPTT to the list of injectable ACPI tables. Fix bug which kept the code from using the processor node as intended in v2, caused by misuse of git rebase/fixup. v1->v2: The parser keys off the acpi_pptt_processor node to determine unique cache's rather than the acpi_pptt_cache referenced by the processor node. This allows PPTT tables which "share" cache nodes across cpu nodes despite not being a shared cache. Normalize the socket, cluster and thread mapping so that they match linux's traditional mapping for the physical id, and thread id. Adding explicit scheduler knowledge of clusters (rather than just their cache sharing attributes) is a subject for a future patch. Jeremy Linton (9): ACPICA: Add additional PPTT flags for cache properties ACPI/PPTT: Add Processor Properties Topology Table parsing arm64/acpi: Create arch specific cpu to acpi id helper ACPI: Enable PPTT support on ARM64 drivers: base: cacheinfo: arm64: Add support for ACPI based firmware tables ACPI/PPTT: Add topology parsing code arm64: Topology, rename cluster_id arm64: topology: Enable ACPI/PPTT based CPU topology. ACPI: Add PPTT to injectable table list arch/arm64/Kconfig | 1 + arch/arm64/include/asm/acpi.h | 4 + arch/arm64/include/asm/topology.h | 4 +- arch/arm64/kernel/cacheinfo.c | 23 +- arch/arm64/kernel/topology.c | 74 ++++- drivers/acpi/Kconfig | 3 + drivers/acpi/Makefile | 1 + drivers/acpi/pptt.c | 570 ++++++++++++++++++++++++++++++++++++++ drivers/acpi/tables.c | 3 +- drivers/base/cacheinfo.c | 17 +- include/acpi/actbl1.h | 14 + include/linux/cacheinfo.h | 11 +- include/linux/topology.h | 2 + 13 files changed, 697 insertions(+), 30 deletions(-) create mode 100644 drivers/acpi/pptt.c -- 2.13.5 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html