[PATCH RFC 0/2] Add basic generic ACPI soc driver

[PATCH RFC 0/2] Add basic generic ACPI soc driver

[John Garry]

A requirement has come up recently to be able to read system SoC packages
identifiers from userspace [0].

For device tree FW-based systems, this would be quite straightforward, in
that we could add a soc driver for that system and use the DT model
identifier as the soc id - that's how most soc drivers seem to do it.

For ACPI-based systems, the only place I know to get (put) such SoC
information is in the PPTT, specifically the ID Type Structure for a
processor package node. A processor package node describes a physical
boundary of a processor topology.

The ACPI spec does not declare how the fields in this structure must be
used, however it does provide pretty clear examples, which I would expect
most implementers to follow. As such, I try to solve the problem in 2
parts:
- Add ACPI PPTT API to get opaque package structure
- Add basic ACPI generic soc driver, which can interpret the fields
  for known platforms to fill in the ID Type Structure as per example
  in the spec.

So I'm hoping here for some comments on this approach - hence the RFC.
I've cc'ed some folks which may have suggestions.

[0] https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/ ,
    https://lore.kernel.org/linux-arm-kernel/1579876505-113251-1-git-send-email-john.garry@huawei.com/

John Garry (2):
  ACPI/PPTT: Add acpi_pptt_get_package_info() API
  soc: Add a basic ACPI generic driver

 drivers/acpi/pptt.c        |  81 +++++++++++++++++++++++++++++
 drivers/soc/Makefile       |   1 +
 drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h       |  13 +++++
 4 files changed, 197 insertions(+)
 create mode 100644 drivers/soc/acpi_generic.c

-- 
2.17.1

[PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[John Garry]

The ACPI PPTT ID structure (see 6.2 spec, section 5.2.29.3) allows the
vendor to provide an identifier (or vendor specific part number) for a
particular processor hierarchy node structure. That may be a processor
identifier for a processor node, or some chip identifier for a processor
package node.

In some circumstances it can be useful to learn the SoC package identifiers
in the system. An example is in [0], where the userspace perf tool needs
to know the SoC identifier for certain per-SoC event matching. So for this
purpose, add an API to get ID structure members for a processor package
node index, which may be used by some driver to expose this info to
userspace.

The ID structure table has a number of fields, which are left open to
interpretation per implementation. However the spec does provide reference
examples of how the fields could be used. As such, just provide the
table fields directly in the API, which the caller may interpret (probably
as per spec example).

https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/acpi/pptt.c  | 81 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h | 13 +++++++
 2 files changed, 94 insertions(+)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index f31544d3656e..ea4ed6300d0b 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -760,3 +760,84 @@ int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
 	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
 					  ACPI_PPTT_ACPI_IDENTICAL);
 }
+
+/**
+ * acpi_pptt_get_package_info() - Get processor package information
+ * @index: Index into processor package
+ * @info: Pointer to structure to fill in processor package info
+ *
+ * For a particular processor package index, fill in the acpi_pptt_package_info
+ * structure.
+ *
+ * Return: -ENOENT if the PPTT or processor package index doesn't exist,
+ *	   -EINVAL for invalid arguments, 0 for success.
+ */
+int acpi_pptt_get_package_info(int index, struct acpi_pptt_package_info *info)
+{
+	struct acpi_subtable_header *entry;
+	struct acpi_table_header *table;
+	unsigned long table_end;
+	acpi_status status;
+	int ret, count = 0;
+
+	if (!info)
+		return -EINVAL;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		acpi_pptt_warn_missing();
+		return -ENOENT;
+	}
+
+	table_end = (unsigned long)table + table->length;
+	entry = ACPI_ADD_PTR(struct acpi_subtable_header, table,
+			     sizeof(struct acpi_table_pptt));
+
+	ret = -ENOENT;
+	while (entry) {
+		struct acpi_pptt_processor *cpu_node;
+
+		cpu_node = (struct acpi_pptt_processor *)entry;
+
+		if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
+		    cpu_node->flags & ACPI_PPTT_PHYSICAL_PACKAGE) {
+			int cnt = cpu_node->number_of_priv_resources;
+			int i;
+
+			for (i = 0; i < cnt; i++) {
+				struct acpi_subtable_header *r;
+
+				r = acpi_get_pptt_resource(table, cpu_node, i);
+
+				if (r->type == ACPI_PPTT_TYPE_ID &&
+				    count == index) {
+					struct acpi_pptt_id *id;
+
+					id = (struct acpi_pptt_id *)r;
+					info->LEVEL_2_ID =
+						le64_to_cpu(id->level2_id);
+					info->vendor_id =
+						le32_to_cpu(id->vendor_id);
+
+					ret = 0;
+					goto out;
+				}
+
+				if (r->type == ACPI_PPTT_TYPE_ID)
+					count++;
+			}
+		}
+
+		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
+				     entry->length);
+		if ((unsigned long)entry >= table_end)
+			break;
+	}
+
+out:
+	acpi_put_table(table);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(acpi_pptt_get_package_info);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f37a7d5fa77..0a911a298731 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1268,13 +1268,26 @@ static inline int lpit_read_residency_count_address(u64 *address)
 }
 #endif
 
+struct acpi_pptt_package_info {
+	u64 LEVEL_2_ID;
+	u32 vendor_id;
+};
+
 #ifdef CONFIG_ACPI_PPTT
 int acpi_pptt_cpu_is_thread(unsigned int cpu);
 int find_acpi_cpu_topology(unsigned int cpu, int level);
 int find_acpi_cpu_topology_package(unsigned int cpu);
 int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
 int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
+int acpi_pptt_get_package_info(int index, struct acpi_pptt_package_info *info);
 #else
+static inline int acpi_pptt_get_package_info(int index,
+					     struct acpi_pptt_package_info *info);
+{
+	return -EINVAL;
+
+}
+
 static inline int acpi_pptt_cpu_is_thread(unsigned int cpu)
 {
 	return -EINVAL;
-- 
2.17.1

[PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

Add a generic driver for platforms which populate their ACPI PPTT
processor package ID Type Structure according to suggestion in the ACPI
spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.

The soc_id is from member LEVEL_2_ID.

For this, we need to use a whitelist of platforms which are known to
populate the structure as suggested.

For now, only the vendor and soc_id fields are exposed.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/soc/Makefile       |   1 +
 drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 drivers/soc/acpi_generic.c

diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 8b49d782a1ab..2a59a30a22cd 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -3,6 +3,7 @@
 # Makefile for the Linux Kernel SOC specific device drivers.
 #
 
+obj-$(CONFIG_ACPI_PPTT)		+= acpi_generic.o
 obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
 obj-$(CONFIG_SOC_ASPEED)	+= aspeed/
 obj-$(CONFIG_ARCH_AT91)		+= atmel/
diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
new file mode 100644
index 000000000000..34a1f5f8e063
--- /dev/null
+++ b/drivers/soc/acpi_generic.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) John Garry, john.garry@huawei.com
+ */
+
+#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
+
+#include <linux/acpi.h>
+#include <linux/sys_soc.h>
+
+/*
+ * Known platforms that fill in PPTT package ID structures according to
+ * ACPI spec examples, that being:
+ * - Custom driver attribute is in ID Type Structure VENDOR_ID member
+ * - SoC id is in ID Type Structure LEVEL_2_ID member
+ *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
+ */
+static struct acpi_platform_list plat_list[] = {
+	{"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
+	{ } /* End */
+};
+
+struct acpi_generic_soc_struct {
+	struct soc_device_attribute dev_attr;
+	u32 vendor;
+};
+
+static ssize_t vendor_show(struct device *dev,
+			   struct device_attribute *attr,
+			   char *buf)
+{
+	struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
+	u8 vendor_id[5] = {};
+
+	*(u32 *)vendor_id = soc->vendor;
+
+	return sprintf(buf, "%s\n", vendor_id);
+}
+
+static DEVICE_ATTR_RO(vendor);
+
+static __init int soc_acpi_generic_init(void)
+{
+	int index;
+
+	index = acpi_match_platform_list(plat_list);
+	if (index < 0)
+		return -ENOENT;
+
+	index = 0;
+	while (true) {
+		struct acpi_pptt_package_info info;
+
+		if (!acpi_pptt_get_package_info(index, &info)) {
+			struct soc_device_attribute *soc_dev_attr;
+			struct acpi_generic_soc_struct *soc;
+			struct soc_device *soc_dev;
+			u8 soc_id[9] = {};
+
+			*(u64 *)soc_id = info.LEVEL_2_ID;
+
+			soc = kzalloc(sizeof(*soc), GFP_KERNEL);
+			if (!soc)
+				return -ENOMEM;
+
+			soc_dev_attr = &soc->dev_attr;
+			soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
+							 soc_id);
+			if (!soc_dev_attr->soc_id) {
+				kfree(soc);
+				return -ENOMEM;
+			}
+			soc->vendor = info.vendor_id;
+
+			soc_dev = soc_device_register(soc_dev_attr);
+			if (IS_ERR(soc_dev)) {
+				int ret = PTR_ERR(soc_dev);
+
+				pr_info("could not register soc (%d) index=%d\n",
+					ret, index);
+				kfree(soc_dev_attr->soc_id);
+				kfree(soc);
+				return ret;
+			}
+			dev_set_drvdata(soc_device_to_device(soc_dev), soc);
+			device_create_file(soc_device_to_device(soc_dev),
+					   &dev_attr_vendor);
+		} else {
+			break;
+		}
+
+		index++;
+	}
+
+	return 0;
+}
+
+module_init(soc_acpi_generic_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("John Garry <john.garry@huawei.com>");
+MODULE_DESCRIPTION("Generic ACPI soc driver");
-- 
2.17.1

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Greg KH]

On Tue, Jan 28, 2020 at 07:14:19PM +0800, John Garry wrote:
> Add a generic driver for platforms which populate their ACPI PPTT
> processor package ID Type Structure according to suggestion in the ACPI
> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
> 
> The soc_id is from member LEVEL_2_ID.
> 
> For this, we need to use a whitelist of platforms which are known to
> populate the structure as suggested.
> 
> For now, only the vendor and soc_id fields are exposed.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/soc/Makefile       |   1 +
>  drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/acpi_generic.c
> 
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..2a59a30a22cd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for the Linux Kernel SOC specific device drivers.
>  #
>  
> +obj-$(CONFIG_ACPI_PPTT)		+= acpi_generic.o
>  obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
>  obj-$(CONFIG_SOC_ASPEED)	+= aspeed/
>  obj-$(CONFIG_ARCH_AT91)		+= atmel/
> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
> new file mode 100644
> index 000000000000..34a1f5f8e063
> --- /dev/null
> +++ b/drivers/soc/acpi_generic.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) John Garry, john.garry@huawei.com
> + */
> +
> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt

You have a device, why do you need pr_fmt()?

> +
> +#include <linux/acpi.h>
> +#include <linux/sys_soc.h>
> +
> +/*
> + * Known platforms that fill in PPTT package ID structures according to
> + * ACPI spec examples, that being:
> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> + */
> +static struct acpi_platform_list plat_list[] = {
> +	{"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
> +	{ } /* End */
> +};
> +
> +struct acpi_generic_soc_struct {
> +	struct soc_device_attribute dev_attr;
> +	u32 vendor;
> +};
> +
> +static ssize_t vendor_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
> +	u8 vendor_id[5] = {};
> +
> +	*(u32 *)vendor_id = soc->vendor;
> +
> +	return sprintf(buf, "%s\n", vendor_id);
> +}
> +
> +static DEVICE_ATTR_RO(vendor);
> +
> +static __init int soc_acpi_generic_init(void)
> +{
> +	int index;
> +
> +	index = acpi_match_platform_list(plat_list);
> +	if (index < 0)
> +		return -ENOENT;
> +
> +	index = 0;
> +	while (true) {
> +		struct acpi_pptt_package_info info;
> +
> +		if (!acpi_pptt_get_package_info(index, &info)) {
> +			struct soc_device_attribute *soc_dev_attr;
> +			struct acpi_generic_soc_struct *soc;
> +			struct soc_device *soc_dev;
> +			u8 soc_id[9] = {};
> +
> +			*(u64 *)soc_id = info.LEVEL_2_ID;
> +
> +			soc = kzalloc(sizeof(*soc), GFP_KERNEL);
> +			if (!soc)
> +				return -ENOMEM;
> +
> +			soc_dev_attr = &soc->dev_attr;
> +			soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +							 soc_id);
> +			if (!soc_dev_attr->soc_id) {
> +				kfree(soc);
> +				return -ENOMEM;
> +			}
> +			soc->vendor = info.vendor_id;
> +
> +			soc_dev = soc_device_register(soc_dev_attr);
> +			if (IS_ERR(soc_dev)) {
> +				int ret = PTR_ERR(soc_dev);
> +
> +				pr_info("could not register soc (%d) index=%d\n",
> +					ret, index);

pr_err()?

And shouldn't the core print out the error, not the person who calls it?


> +				kfree(soc_dev_attr->soc_id);
> +				kfree(soc);
> +				return ret;
> +			}
> +			dev_set_drvdata(soc_device_to_device(soc_dev), soc);
> +			device_create_file(soc_device_to_device(soc_dev),
> +					   &dev_attr_vendor);

You just raced with userspace and lost.  Use the built-in api that I
made _just_ because of SOC drivers to do this correctly.

thanks,

greg k-h

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[Sudeep Holla]

On Tue, Jan 28, 2020 at 07:14:18PM +0800, John Garry wrote:
> The ACPI PPTT ID structure (see 6.2 spec, section 5.2.29.3) allows the
> vendor to provide an identifier (or vendor specific part number) for a
> particular processor hierarchy node structure. That may be a processor
> identifier for a processor node, or some chip identifier for a processor
> package node.
>

Unfortunately, there were plans to deprecate this in favour of the new
SOC_ID SMCCC API[1]. I am not sure if you or anyone in your company have
access to UEFI ASWG mantis where you can look for the ECR for the PPTT
Type 2 deprecation. I understand it's not ideal, but we need to converge,
please take a look at both before further discussion.

I personally would not prefer to add the support when I know it is getting
deprecated. I am not sure on kernel community policy on the same.


[...]

>
> The ID structure table has a number of fields, which are left open to
> interpretation per implementation. However the spec does provide reference
> examples of how the fields could be used. As such, just provide the
> table fields directly in the API, which the caller may interpret (probably
> as per spec example).
>

The "open for interpretation" part is why it's not being favoured anymore
by silicon vendors as OEM/ODMs can override the same.

> https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/
>
Ah, there's already quite a lot of dependency built for this feature :(

--
Regards,
Sudeep

[1] https://developer.arm.com/docs/den0028/c

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Arnd Bergmann]

On Tue, Jan 28, 2020 at 12:18 PM John Garry <john.garry@huawei.com> wrote:
>
> Add a generic driver for platforms which populate their ACPI PPTT
> processor package ID Type Structure according to suggestion in the ACPI
> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>
> The soc_id is from member LEVEL_2_ID.
>
> For this, we need to use a whitelist of platforms which are known to
> populate the structure as suggested.
>
> For now, only the vendor and soc_id fields are exposed.
>
> Signed-off-by: John Garry <john.garry@huawei.com>

Would it be possible to make this the root device for all on-chip devices
to correctly reflect the hierarchy inside of the soc?

> +/*
> + * Known platforms that fill in PPTT package ID structures according to
> + * ACPI spec examples, that being:
> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> + */
> +static struct acpi_platform_list plat_list[] = {
> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
> +       { } /* End */
> +};

That matches a single machine, right? It doesn't seem very generic
that way.

> +struct acpi_generic_soc_struct {
> +       struct soc_device_attribute dev_attr;
> +       u32 vendor;
> +};
> +
> +static ssize_t vendor_show(struct device *dev,
> +                          struct device_attribute *attr,
> +                          char *buf)
> +{
> +       struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
> +       u8 vendor_id[5] = {};
> +
> +       *(u32 *)vendor_id = soc->vendor;
> +
> +       return sprintf(buf, "%s\n", vendor_id);
> +}

I'd rather not see nonstandard attributes in a "generic" driver at
all. Maybe the
you can simply concatenate the vendor and  LEVEL_2_ID into a single string
here?

> +                       soc = kzalloc(sizeof(*soc), GFP_KERNEL);
> +                       if (!soc)
> +                               return -ENOMEM;
> +
> +                       soc_dev_attr = &soc->dev_attr;
> +                       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +                                                        soc_id);

On the other hand, it would make sense to fill out additional fields here.
You have already matched the name of the board from the
acpi_platform_list, so there are two strings available that could be put
into the "machine" field, and it would make sense to fill out "family" with
something that identifies it as coming from ACPI PPTT data.

             Arnd

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

Hi Greg,

>> +
>> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
> 
> You have a device, why do you need pr_fmt()?
> 

The only print in the code can be removed, below, so I need not worry 
about this, i.e. remove it.

>> +
>> +#include <linux/acpi.h>
>> +#include <linux/sys_soc.h>
>> +
>> +/*
>> + * Known platforms that fill in PPTT package ID structures according to
>> + * ACPI spec examples, that being:
>> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
>> + * - SoC id is in ID Type Structure LEVEL_2_ID member
>> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
>> + */
>> +static struct acpi_platform_list plat_list[] = {
>> +	{"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
>> +	{ } /* End */
>> +};
>> +
>> +struct acpi_generic_soc_struct {
>> +	struct soc_device_attribute dev_attr;
>> +	u32 vendor;
>> +};
>> +
>> +static ssize_t vendor_show(struct device *dev,
>> +			   struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
>> +	u8 vendor_id[5] = {};
>> +
>> +	*(u32 *)vendor_id = soc->vendor;
>> +
>> +	return sprintf(buf, "%s\n", vendor_id);
>> +}
>> +
>> +static DEVICE_ATTR_RO(vendor);
>> +
>> +static __init int soc_acpi_generic_init(void)
>> +{
>> +	int index;
>> +
>> +	index = acpi_match_platform_list(plat_list);
>> +	if (index < 0)
>> +		return -ENOENT;
>> +
>> +	index = 0;
>> +	while (true) {
>> +		struct acpi_pptt_package_info info;
>> +
>> +		if (!acpi_pptt_get_package_info(index, &info)) {
>> +			struct soc_device_attribute *soc_dev_attr;
>> +			struct acpi_generic_soc_struct *soc;
>> +			struct soc_device *soc_dev;
>> +			u8 soc_id[9] = {};
>> +
>> +			*(u64 *)soc_id = info.LEVEL_2_ID;
>> +
>> +			soc = kzalloc(sizeof(*soc), GFP_KERNEL);
>> +			if (!soc)
>> +				return -ENOMEM;
>> +
>> +			soc_dev_attr = &soc->dev_attr;
>> +			soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
>> +							 soc_id);
>> +			if (!soc_dev_attr->soc_id) {
>> +				kfree(soc);
>> +				return -ENOMEM;
>> +			}
>> +			soc->vendor = info.vendor_id;
>> +
>> +			soc_dev = soc_device_register(soc_dev_attr);
>> +			if (IS_ERR(soc_dev)) {
>> +				int ret = PTR_ERR(soc_dev);
>> +
>> +				pr_info("could not register soc (%d) index=%d\n",
>> +					ret, index);
> 
> pr_err()?

Yes, more appropriate.

> 
> And shouldn't the core print out the error, not the person who calls it?

Sure, that would sounds reasonable, but I just wanted to get the index 
at which we fail. I could live without it.

> 
> 
>> +				kfree(soc_dev_attr->soc_id);
>> +				kfree(soc);
>> +				return ret;
>> +			}
>> +			dev_set_drvdata(soc_device_to_device(soc_dev), soc);
>> +			device_create_file(soc_device_to_device(soc_dev),
>> +					   &dev_attr_vendor);
> 
> You just raced with userspace and lost.  Use the built-in api that I
> made _just_ because of SOC drivers to do this correctly.
> 

Fine, there is the soc device custom attr group which I can use. But, as 
Arnd said, maybe we can drop this custom file.

Cheers,
John

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[John Garry]

On 28/01/2020 12:34, Sudeep Holla wrote:

Hi Sudeep,

> On Tue, Jan 28, 2020 at 07:14:18PM +0800, John Garry wrote:
>> The ACPI PPTT ID structure (see 6.2 spec, section 5.2.29.3) allows the
>> vendor to provide an identifier (or vendor specific part number) for a
>> particular processor hierarchy node structure. That may be a processor
>> identifier for a processor node, or some chip identifier for a processor
>> package node.
>>
> 
> Unfortunately, there were plans to deprecate this in favour of the new
> SOC_ID SMCCC API[1]. I am not sure if you or anyone in your company have
> access to UEFI ASWG mantis where you can look for the ECR for the PPTT
> Type 2 deprecation.

I wasn't aware and I can't get access...

Personally I would rather PPTT ID structure have a fixed field 
definition in future spec versions, rather than deprecate.

 From checking here, nobody has even used it (properly) for processor 
package nodes:
https://github.com/tianocore/edk2-platforms/tree/master/Platform

  I understand it's not ideal, but we need to converge,
> please take a look at both before further discussion.

I can only check the SMCCC extension which you pointed me at.

> 
> I personally would not prefer to add the support when I know it is getting
> deprecated. I am not sure on kernel community policy on the same.

So I need a generic solution for this, as my userspace tool requires a 
generic solution.

> 
> 
> [...]
> 
>>
>> The ID structure table has a number of fields, which are left open to
>> interpretation per implementation. However the spec does provide reference
>> examples of how the fields could be used. As such, just provide the
>> table fields directly in the API, which the caller may interpret (probably
>> as per spec example).
>>
> 
> The "open for interpretation" part is why it's not being favoured anymore
> by silicon vendors as OEM/ODMs can override the same.
> 
>> https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/
>>
> Ah, there's already quite a lot of dependency built for this feature :(

Not really. It's only an RFC ATM, and my requirement is a sysfs file to 
read the SoC id(s) (under ACPI FW). So I would still expect to be able 
to support this from the SMCCC extension method.

> 
> --
> Regards,
> Sudeep
> 
> [1] https://developer.arm.com/docs/den0028/c
> .
> 

Cheers,
John

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

On 28/01/2020 12:50, Arnd Bergmann wrote:

Hi Arnd,

> On Tue, Jan 28, 2020 at 12:18 PM John Garry <john.garry@huawei.com> wrote:
>>
>> Add a generic driver for platforms which populate their ACPI PPTT
>> processor package ID Type Structure according to suggestion in the ACPI
>> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>>
>> The soc_id is from member LEVEL_2_ID.
>>
>> For this, we need to use a whitelist of platforms which are known to
>> populate the structure as suggested.
>>
>> For now, only the vendor and soc_id fields are exposed.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> Would it be possible to make this the root device for all on-chip devices
> to correctly reflect the hierarchy inside of the soc?

I don't think so. The information about the SoC is got from the PPTT, 
which only describes processors, caches, and physical package 
boundaries. It doesn't include references to on-chip devices.

Having said that (and unrelated to this series), we could add 
/sys/devices/system/soc folder, similar to node folder.

> 
>> +/*
>> + * Known platforms that fill in PPTT package ID structures according to
>> + * ACPI spec examples, that being:
>> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
>> + * - SoC id is in ID Type Structure LEVEL_2_ID member
>> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
>> + */
>> +static struct acpi_platform_list plat_list[] = {
>> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
>> +       { } /* End */
>> +};
> 
> That matches a single machine, right? It doesn't seem very generic
> that way.

Yes :) The problem is that the PPTT ID structure is open to use how the 
implementer wants, so we can't assume everything/anything implemented 
according to the spec examples. Maybe we could call it type1 or 
something like that for platforms which did use the convention in the 
spec example.

> 
>> +struct acpi_generic_soc_struct {
>> +       struct soc_device_attribute dev_attr;
>> +       u32 vendor;
>> +};
>> +
>> +static ssize_t vendor_show(struct device *dev,
>> +                          struct device_attribute *attr,
>> +                          char *buf)
>> +{
>> +       struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
>> +       u8 vendor_id[5] = {};
>> +
>> +       *(u32 *)vendor_id = soc->vendor;
>> +
>> +       return sprintf(buf, "%s\n", vendor_id);
>> +}
> 
> I'd rather not see nonstandard attributes in a "generic" driver at
> all. Maybe the
> you can simply concatenate the vendor and  LEVEL_2_ID into a single string
> here?

I actually don't really require the vendor attr. And since "vendor" is 
not in the set of standard soc driver attrs, it can just be omitted.

> 
>> +                       soc = kzalloc(sizeof(*soc), GFP_KERNEL);
>> +                       if (!soc)
>> +                               return -ENOMEM;
>> +
>> +                       soc_dev_attr = &soc->dev_attr;
>> +                       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
>> +                                                        soc_id);
> 
> On the other hand, it would make sense to fill out additional fields here.
> You have already matched the name of the board from the
> acpi_platform_list, so there are two strings available that could be put
> into the "machine" field, and it would make sense to fill out "family" with
> something that identifies it as coming from ACPI PPTT data.

OK, maybe the ones you suggested could be added. I did just want to 
start out with a minimal sets of files, especially since we don't always 
have a direct mapping between soc driver attrs and this PPTT ID structure.

Thanks,
John

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[Sudeep Holla]

On Tue, Jan 28, 2020 at 02:04:19PM +0000, John Garry wrote:
> On 28/01/2020 12:34, Sudeep Holla wrote:
>
> Hi Sudeep,
>
> > On Tue, Jan 28, 2020 at 07:14:18PM +0800, John Garry wrote:
> > > The ACPI PPTT ID structure (see 6.2 spec, section 5.2.29.3) allows the
> > > vendor to provide an identifier (or vendor specific part number) for a
> > > particular processor hierarchy node structure. That may be a processor
> > > identifier for a processor node, or some chip identifier for a processor
> > > package node.
> > >
> >
> > Unfortunately, there were plans to deprecate this in favour of the new
> > SOC_ID SMCCC API[1]. I am not sure if you or anyone in your company have
> > access to UEFI ASWG mantis where you can look for the ECR for the PPTT
> > Type 2 deprecation.
>
> I wasn't aware and I can't get access...
>

I can understand, it is not well published/advertised.

> Personally I would rather PPTT ID structure have a fixed field definition in
> future spec versions, rather than deprecate.
>
> From checking here, nobody has even used it (properly) for processor package
> nodes:
> https://github.com/tianocore/edk2-platforms/tree/master/Platform
>

Yes, that was one of the things we looked at when we started with SOC_ID
SMCCC API and proposal to deprecate PPTT Type 2 table.

> > I understand it's not ideal, but we need to converge,
> > please take a look at both before further discussion.
>
> I can only check the SMCCC extension which you pointed me at.
>

Sure, that will at-least give you what SMCCC SOC_ID API looks like.

> >
> > I personally would not prefer to add the support when I know it is getting
> > deprecated. I am not sure on kernel community policy on the same.
>
> So I need a generic solution for this, as my userspace tool requires a
> generic solution.
>

Yes I agree on the generic solution and the soc driver you have proposed
in the patch. No objections there, just the source of the information
needs to be changed. Instead of ACPI PPTT Type 2 table, it needs to be
SOC_ID SMCCC v1.2 API

> >
> >
> > [...]
> >
> > >
> > > The ID structure table has a number of fields, which are left open to
> > > interpretation per implementation. However the spec does provide reference
> > > examples of how the fields could be used. As such, just provide the
> > > table fields directly in the API, which the caller may interpret (probably
> > > as per spec example).
> > >
> >
> > The "open for interpretation" part is why it's not being favoured anymore
> > by silicon vendors as OEM/ODMs can override the same.
> >
> > > https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/
> > >
> > Ah, there's already quite a lot of dependency built for this feature :(
>
> Not really. It's only an RFC ATM, and my requirement is a sysfs file to read
> the SoC id(s) (under ACPI FW). So I would still expect to be able to support
> this from the SMCCC extension method.
>

As mentioned above, yes the driver would remain almost same for SMCCC
SOC_ID support too. The main point was: do we need to add support to
PPTT Type 2 entry when we know there is proposal to deprecate it. I
would at-least wait to see progress on that until I would add this to
the kernel.

--
Regards,
Sudeep

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Sudeep Holla]

(commenting on other parts though I am not sure if we want to add this
despite it being deprecated)

On Tue, Jan 28, 2020 at 07:14:19PM +0800, John Garry wrote:
> Add a generic driver for platforms which populate their ACPI PPTT
> processor package ID Type Structure according to suggestion in the ACPI
> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>
> The soc_id is from member LEVEL_2_ID.
>
> For this, we need to use a whitelist of platforms which are known to
> populate the structure as suggested.
>
> For now, only the vendor and soc_id fields are exposed.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/soc/Makefile       |   1 +
>  drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/acpi_generic.c
>
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..2a59a30a22cd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for the Linux Kernel SOC specific device drivers.
>  #
>
> +obj-$(CONFIG_ACPI_PPTT)		+= acpi_generic.o
>  obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
>  obj-$(CONFIG_SOC_ASPEED)	+= aspeed/
>  obj-$(CONFIG_ARCH_AT91)		+= atmel/
> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
> new file mode 100644
> index 000000000000..34a1f5f8e063
> --- /dev/null
> +++ b/drivers/soc/acpi_generic.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) John Garry, john.garry@huawei.com
> + */
> +
> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/sys_soc.h>
> +
> +/*
> + * Known platforms that fill in PPTT package ID structures according to
> + * ACPI spec examples, that being:
> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> + */
> +static struct acpi_platform_list plat_list[] = {
> +	{"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},

What do you want to match this ? The same silicon can end up with
different OEMs and this list just blows up soon for single SoC if
used by different OEM/ODMs. I assume we get all the required info
from the Type 2 table entry and hence can just rely on that. If
PPTT has type 2 entry, just initialise this soc driver and expose
the relevant information from the table entry.

--
Regards,
Sudeep

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

On 28/01/2020 15:20, Sudeep Holla wrote:
> (commenting on other parts though I am not sure if we want to add this
> despite it being deprecated)
> 
> On Tue, Jan 28, 2020 at 07:14:19PM +0800, John Garry wrote:
>> Add a generic driver for platforms which populate their ACPI PPTT
>> processor package ID Type Structure according to suggestion in the ACPI
>> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>>
>> The soc_id is from member LEVEL_2_ID.
>>
>> For this, we need to use a whitelist of platforms which are known to
>> populate the structure as suggested.
>>
>> For now, only the vendor and soc_id fields are exposed.
>>
>> Signed-off-by: John Garry<john.garry@huawei.com>
>> ---
>>   drivers/soc/Makefile       |   1 +
>>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 103 insertions(+)
>>   create mode 100644 drivers/soc/acpi_generic.c
>>
>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>> index 8b49d782a1ab..2a59a30a22cd 100644
>> --- a/drivers/soc/Makefile
>> +++ b/drivers/soc/Makefile
>> @@ -3,6 +3,7 @@
>>   # Makefile for the Linux Kernel SOC specific device drivers.
>>   #
>>
>> +obj-$(CONFIG_ACPI_PPTT)		+= acpi_generic.o
>>   obj-$(CONFIG_ARCH_ACTIONS)	+= actions/
>>   obj-$(CONFIG_SOC_ASPEED)	+= aspeed/
>>   obj-$(CONFIG_ARCH_AT91)		+= atmel/
>> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
>> new file mode 100644
>> index 000000000000..34a1f5f8e063
>> --- /dev/null
>> +++ b/drivers/soc/acpi_generic.c
>> @@ -0,0 +1,102 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) John Garry,john.garry@huawei.com
>> + */
>> +
>> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/sys_soc.h>
>> +
>> +/*
>> + * Known platforms that fill in PPTT package ID structures according to
>> + * ACPI spec examples, that being:
>> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
>> + * - SoC id is in ID Type Structure LEVEL_2_ID member
>> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
>> + */
>> +static struct acpi_platform_list plat_list[] = {
>> +	{"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},

Hi Sudeep,

> What do you want to match this ? The same silicon can end up with
> different OEMs and this list just blows up soon for single SoC if
> used by different OEM/ODMs. I assume we get all the required info
> from the Type 2 table entry and hence can just rely on that. If
> PPTT has type 2 entry, just initialise this soc driver and expose
> the relevant information from the table entry.

As before, the LEVEL_1_ID and LEVEL_2_ID table members are too open to 
interpretation in the spec to generate a consistent form soc_id and 
family_id for all platforms.

As such, I was trying to limit to known PPTT implementations and how 
they should be interpreted. Obviously that's *far* from ideal.

So what's your idea? Just always put LEVEL_1_ID and LEVEL_2_ID in soc 
driver family_id and soc_id fields, respectively?

Thanks,
John

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Sudeep Holla]

On Tue, Jan 28, 2020 at 03:59:15PM +0000, John Garry wrote:
> On 28/01/2020 15:20, Sudeep Holla wrote:
>

[...]

> Hi Sudeep,
>
> > What do you want to match this ? The same silicon can end up with
> > different OEMs and this list just blows up soon for single SoC if
> > used by different OEM/ODMs. I assume we get all the required info
> > from the Type 2 table entry and hence can just rely on that. If
> > PPTT has type 2 entry, just initialise this soc driver and expose
> > the relevant information from the table entry.
>
> As before, the LEVEL_1_ID and LEVEL_2_ID table members are too open to
> interpretation in the spec to generate a consistent form soc_id and
> family_id for all platforms.
>

One of the argument I was making during evolution of SOC_ID is to have
IDs like LEVEL_1_ID and LEVEL_2_ID in PPTT as they are 8 bytes long and
can just be a string that is self-sufficient and can be exposed to user
space as is instead of having to do some interpretation in the kernel.
Remember this is ACPI table entry and is designed to work with multiple
OS, we can at-least expect the strings to be as self-explanatory and
need no further decoding in the kernel.

> As such, I was trying to limit to known PPTT implementations and how they
> should be interpreted. Obviously that's *far* from ideal.
>

I am saying not to take that path and just throw the strings as is at
the user. If OEM/ODMs are serious about suggesting user-space to make
use of it, they better put sane value there and don't expect kernel to
do interpretation for them.

> So what's your idea? Just always put LEVEL_1_ID and LEVEL_2_ID in soc driver
> family_id and soc_id fields, respectively?
>

Yes, that's my opinion. It gets messy soon if kernel tries to interpret
this for OEM/ODM, they must better get it right if they are serious about
it.

--
Regards,
Sudeep

Re: [PATCH RFC 0/2] Add basic generic ACPI soc driver

[Jeremy Linton]

Hi,

On 1/28/20 5:14 AM, John Garry wrote:
> A requirement has come up recently to be able to read system SoC packages
> identifiers from userspace [0].
> 
> For device tree FW-based systems, this would be quite straightforward, in
> that we could add a soc driver for that system and use the DT model
> identifier as the soc id - that's how most soc drivers seem to do it.
> 
> For ACPI-based systems, the only place I know to get (put) such SoC
> information is in the PPTT, specifically the ID Type Structure for a
> processor package node. A processor package node describes a physical
> boundary of a processor topology.

Well presumably that is one of the use cases for DMI, which has fields 
for the processor/socket as well as the machine vendor.

But, quickly looking at the use case, I can't help but think you don't 
really want any of the above, or the PPTT id. It seems the mapping 
should actually be tied directly to the uncore PMU definition, rather 
than a soc/machine/whatever identifier. Which would imply keying off one 
of the ACPI object identifiers for the PMU itself.


> 
> The ACPI spec does not declare how the fields in this structure must be
> used, however it does provide pretty clear examples, which I would expect
> most implementers to follow. As such, I try to solve the problem in 2
> parts:
> - Add ACPI PPTT API to get opaque package structure
> - Add basic ACPI generic soc driver, which can interpret the fields
>    for known platforms to fill in the ID Type Structure as per example
>    in the spec.
> 
> So I'm hoping here for some comments on this approach - hence the RFC.
> I've cc'ed some folks which may have suggestions.
> 
> [0] https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/ ,
>      https://lore.kernel.org/linux-arm-kernel/1579876505-113251-1-git-send-email-john.garry@huawei.com/
> 
> John Garry (2):
>    ACPI/PPTT: Add acpi_pptt_get_package_info() API
>    soc: Add a basic ACPI generic driver
> 
>   drivers/acpi/pptt.c        |  81 +++++++++++++++++++++++++++++
>   drivers/soc/Makefile       |   1 +
>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>   include/linux/acpi.h       |  13 +++++
>   4 files changed, 197 insertions(+)
>   create mode 100644 drivers/soc/acpi_generic.c
> 

Re: [PATCH RFC 0/2] Add basic generic ACPI soc driver

[John Garry]

On 28/01/2020 16:56, Jeremy Linton wrote:
> Hi,
> 

Hi Jeremy,

> On 1/28/20 5:14 AM, John Garry wrote:
>> A requirement has come up recently to be able to read system SoC packages
>> identifiers from userspace [0].
>>
>> For device tree FW-based systems, this would be quite straightforward, in
>> that we could add a soc driver for that system and use the DT model
>> identifier as the soc id - that's how most soc drivers seem to do it.
>>
>> For ACPI-based systems, the only place I know to get (put) such SoC
>> information is in the PPTT, specifically the ID Type Structure for a
>> processor package node. A processor package node describes a physical
>> boundary of a processor topology.
> 
> Well presumably that is one of the use cases for DMI, which has fields 
> for the processor/socket as well as the machine vendor.

I did consider DMI, but I want something more generic, i.e. could cover 
embedded/DT systems also.

And I need to double check if DMI really has the info I require. Last 
time I checked, it didn't for my dev board, but I know that some fields 
are simply not filled in.

> 
> But, quickly looking at the use case, I can't help but think you don't 
> really want any of the above, or the PPTT id. It seems the mapping 
> should actually be tied directly to the uncore PMU definition, rather 
> than a soc/machine/whatever identifier. Which would imply keying off one 
> of the ACPI object identifiers for the PMU itself.

So a PMU device (/sys/bus/event_source/devices) does not have a link to 
the ACPI object identifiers or uncore PMU platform device etc.

And even if it did, there is no clear link between that ACPI object and 
the events it supports for that implementation.

Cheers,
John

> 
> 
>>
>> The ACPI spec does not declare how the fields in this structure must be
>> used, however it does provide pretty clear examples, which I would expect
>> most implementers to follow. As such, I try to solve the problem in 2
>> parts:
>> - Add ACPI PPTT API to get opaque package structure
>> - Add basic ACPI generic soc driver, which can interpret the fields
>>    for known platforms to fill in the ID Type Structure as per example
>>    in the spec.
>>
>> So I'm hoping here for some comments on this approach - hence the RFC.
>> I've cc'ed some folks which may have suggestions.
>>
>> [0] 
>> https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/ 
>> ,
>>      
>> https://lore.kernel.org/linux-arm-kernel/1579876505-113251-1-git-send-email-john.garry@huawei.com/ 
>>
>>
>> John Garry (2):
>>    ACPI/PPTT: Add acpi_pptt_get_package_info() API
>>    soc: Add a basic ACPI generic driver
>>
>>   drivers/acpi/pptt.c        |  81 +++++++++++++++++++++++++++++
>>   drivers/soc/Makefile       |   1 +
>>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>>   include/linux/acpi.h       |  13 +++++
>>   4 files changed, 197 insertions(+)
>>   create mode 100644 drivers/soc/acpi_generic.c
>>
> 
> .

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Olof Johansson]

Hi,

On Tue, Jan 28, 2020 at 3:18 AM John Garry <john.garry@huawei.com> wrote:
>
> Add a generic driver for platforms which populate their ACPI PPTT
> processor package ID Type Structure according to suggestion in the ACPI
> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>
> The soc_id is from member LEVEL_2_ID.
>
> For this, we need to use a whitelist of platforms which are known to
> populate the structure as suggested.
>
> For now, only the vendor and soc_id fields are exposed.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/soc/Makefile       |   1 +
>  drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 drivers/soc/acpi_generic.c
>
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..2a59a30a22cd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -3,6 +3,7 @@
>  # Makefile for the Linux Kernel SOC specific device drivers.
>  #
>
> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
>  obj-$(CONFIG_ARCH_ACTIONS)     += actions/
>  obj-$(CONFIG_SOC_ASPEED)       += aspeed/
>  obj-$(CONFIG_ARCH_AT91)                += atmel/

Based on everything I've seen so far, this should go under drivers/acpi instead.

> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
> new file mode 100644
> index 000000000000..34a1f5f8e063
> --- /dev/null
> +++ b/drivers/soc/acpi_generic.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) John Garry, john.garry@huawei.com
> + */
> +
> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/sys_soc.h>
> +
> +/*
> + * Known platforms that fill in PPTT package ID structures according to
> + * ACPI spec examples, that being:
> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> + */
> +static struct acpi_platform_list plat_list[] = {
> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
> +       { } /* End */
> +};

As others have said, this will become a mess over time, and will
require changes for every new platform. Which, unfortunately, is
exactly what ACPI is supposed to provide relief from by making
standardized platforms... standardized.

> +
> +struct acpi_generic_soc_struct {
> +       struct soc_device_attribute dev_attr;
> +       u32 vendor;
> +};
> +
> +static ssize_t vendor_show(struct device *dev,
> +                          struct device_attribute *attr,
> +                          char *buf)
> +{
> +       struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
> +       u8 vendor_id[5] = {};
> +
> +       *(u32 *)vendor_id = soc->vendor;
> +
> +       return sprintf(buf, "%s\n", vendor_id);
> +}
> +
> +static DEVICE_ATTR_RO(vendor);
> +
> +static __init int soc_acpi_generic_init(void)
> +{
> +       int index;
> +
> +       index = acpi_match_platform_list(plat_list);
> +       if (index < 0)
> +               return -ENOENT;
> +
> +       index = 0;
> +       while (true) {
> +               struct acpi_pptt_package_info info;
> +
> +               if (!acpi_pptt_get_package_info(index, &info)) {
> +                       struct soc_device_attribute *soc_dev_attr;
> +                       struct acpi_generic_soc_struct *soc;
> +                       struct soc_device *soc_dev;
> +                       u8 soc_id[9] = {};
> +
> +                       *(u64 *)soc_id = info.LEVEL_2_ID;
> +
> +                       soc = kzalloc(sizeof(*soc), GFP_KERNEL);
> +                       if (!soc)
> +                               return -ENOMEM;
> +
> +                       soc_dev_attr = &soc->dev_attr;
> +                       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
> +                                                        soc_id);
> +                       if (!soc_dev_attr->soc_id) {
> +                               kfree(soc);
> +                               return -ENOMEM;
> +                       }
> +                       soc->vendor = info.vendor_id;
> +
> +                       soc_dev = soc_device_register(soc_dev_attr);
> +                       if (IS_ERR(soc_dev)) {
> +                               int ret = PTR_ERR(soc_dev);
> +
> +                               pr_info("could not register soc (%d) index=%d\n",
> +                                       ret, index);
> +                               kfree(soc_dev_attr->soc_id);
> +                               kfree(soc);
> +                               return ret;
> +                       }
> +                       dev_set_drvdata(soc_device_to_device(soc_dev), soc);
> +                       device_create_file(soc_device_to_device(soc_dev),
> +                                          &dev_attr_vendor);

Hmm, this doesn't look like much of a driver to me. This looks like
the export of an attribute to userspace, and should probably be done
by ACPI core instead of creating an empty driver for it.

This would also solve the whitelist issue -- always export this
property if it's set. If it's wrong, then the platform vendor needs to
fix it up. That's the approach that is used for other aspects of the
standardized platforms, right? We don't want to litter the kernel with
white/blacklists -- that's not a net improvement.


-Olof

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

On 28/01/2020 17:51, Olof Johansson wrote:
> Hi,
> 
> On Tue, Jan 28, 2020 at 3:18 AM John Garry <john.garry@huawei.com> wrote:
>>

Hi Olof,

>> Add a generic driver for platforms which populate their ACPI PPTT
>> processor package ID Type Structure according to suggestion in the ACPI
>> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
>>
>> The soc_id is from member LEVEL_2_ID.
>>
>> For this, we need to use a whitelist of platforms which are known to
>> populate the structure as suggested.
>>
>> For now, only the vendor and soc_id fields are exposed.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/soc/Makefile       |   1 +
>>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 103 insertions(+)
>>   create mode 100644 drivers/soc/acpi_generic.c
>>
>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>> index 8b49d782a1ab..2a59a30a22cd 100644
>> --- a/drivers/soc/Makefile
>> +++ b/drivers/soc/Makefile
>> @@ -3,6 +3,7 @@
>>   # Makefile for the Linux Kernel SOC specific device drivers.
>>   #
>>
>> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
>>   obj-$(CONFIG_ARCH_ACTIONS)     += actions/
>>   obj-$(CONFIG_SOC_ASPEED)       += aspeed/
>>   obj-$(CONFIG_ARCH_AT91)                += atmel/
> 
> Based on everything I've seen so far, this should go under drivers/acpi instead.

soc drivers seem to live in drivers/soc (non-arm32, anyway), so I 
decided on this location. But drivers/acpi would also seem reasonable now.

> 
>> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
>> new file mode 100644
>> index 000000000000..34a1f5f8e063
>> --- /dev/null
>> +++ b/drivers/soc/acpi_generic.c
>> @@ -0,0 +1,102 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) John Garry, john.garry@huawei.com
>> + */
>> +
>> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/sys_soc.h>
>> +
>> +/*
>> + * Known platforms that fill in PPTT package ID structures according to
>> + * ACPI spec examples, that being:
>> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
>> + * - SoC id is in ID Type Structure LEVEL_2_ID member
>> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
>> + */
>> +static struct acpi_platform_list plat_list[] = {
>> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
>> +       { } /* End */
>> +};
> 
> As others have said, this will become a mess over time, and will
> require changes for every new platform. Which, unfortunately, is
> exactly what ACPI is supposed to provide relief from by making
> standardized platforms... standardized.
> 

Right, and I think that it can be dropped. As discussed with Sudeep, I 
was concerned how this PPTT ID structure could be interpreted, and had a 
whitelist as a conservative approach.

>> +
>> +struct acpi_generic_soc_struct {
>> +       struct soc_device_attribute dev_attr;
>> +       u32 vendor;
>> +};
>> +
>> +static ssize_t vendor_show(struct device *dev,
>> +                          struct device_attribute *attr,
>> +                          char *buf)
>> +{
>> +       struct acpi_generic_soc_struct *soc = dev_get_drvdata(dev);
>> +       u8 vendor_id[5] = {};
>> +
>> +       *(u32 *)vendor_id = soc->vendor;
>> +
>> +       return sprintf(buf, "%s\n", vendor_id);
>> +}
>> +
>> +static DEVICE_ATTR_RO(vendor);
>> +
>> +static __init int soc_acpi_generic_init(void)
>> +{
>> +       int index;
>> +
>> +       index = acpi_match_platform_list(plat_list);
>> +       if (index < 0)
>> +               return -ENOENT;
>> +
>> +       index = 0;
>> +       while (true) {
>> +               struct acpi_pptt_package_info info;
>> +
>> +               if (!acpi_pptt_get_package_info(index, &info)) {
>> +                       struct soc_device_attribute *soc_dev_attr;
>> +                       struct acpi_generic_soc_struct *soc;
>> +                       struct soc_device *soc_dev;
>> +                       u8 soc_id[9] = {};
>> +
>> +                       *(u64 *)soc_id = info.LEVEL_2_ID;
>> +
>> +                       soc = kzalloc(sizeof(*soc), GFP_KERNEL);
>> +                       if (!soc)
>> +                               return -ENOMEM;
>> +
>> +                       soc_dev_attr = &soc->dev_attr;
>> +                       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s",
>> +                                                        soc_id);
>> +                       if (!soc_dev_attr->soc_id) {
>> +                               kfree(soc);
>> +                               return -ENOMEM;
>> +                       }
>> +                       soc->vendor = info.vendor_id;
>> +
>> +                       soc_dev = soc_device_register(soc_dev_attr);
>> +                       if (IS_ERR(soc_dev)) {
>> +                               int ret = PTR_ERR(soc_dev);
>> +
>> +                               pr_info("could not register soc (%d) index=%d\n",
>> +                                       ret, index);
>> +                               kfree(soc_dev_attr->soc_id);
>> +                               kfree(soc);
>> +                               return ret;
>> +                       }
>> +                       dev_set_drvdata(soc_device_to_device(soc_dev), soc);
>> +                       device_create_file(soc_device_to_device(soc_dev),
>> +                                          &dev_attr_vendor);
> 
> Hmm, this doesn't look like much of a driver to me. This looks like
> the export of an attribute to userspace, and should probably be done
> by ACPI core instead of creating an empty driver for it.

OK, but I'm thinking that having a soc driver can be useful as it is 
common to DT, and so userspace only has to check a single location. And 
the soc driver can also cover multiple-chip systems without have to 
reinvent that code for ACPI core. And it saves adding a new ABI.

> 
> This would also solve the whitelist issue -- always export this
> property if it's set. If it's wrong, then the platform vendor needs to
> fix it up. That's the approach that is used for other aspects of the
> standardized platforms, right? We don't want to litter the kernel with
> white/blacklists -- that's not a net improvement.

Agreed.

> 
> 
> -Olof
> .

Thanks,
John

> 

Re: [PATCH RFC 0/2] Add basic generic ACPI soc driver

[Jeremy Linton]

Hi,

On 1/28/20 11:28 AM, John Garry wrote:
> On 28/01/2020 16:56, Jeremy Linton wrote:
>> Hi,
>>
> 
> Hi Jeremy,
> 
>> On 1/28/20 5:14 AM, John Garry wrote:
>>> A requirement has come up recently to be able to read system SoC 
>>> packages
>>> identifiers from userspace [0].
>>>
>>> For device tree FW-based systems, this would be quite 
>>> straightforward, in
>>> that we could add a soc driver for that system and use the DT model
>>> identifier as the soc id - that's how most soc drivers seem to do it.
>>>
>>> For ACPI-based systems, the only place I know to get (put) such SoC
>>> information is in the PPTT, specifically the ID Type Structure for a
>>> processor package node. A processor package node describes a physical
>>> boundary of a processor topology.
>>
>> Well presumably that is one of the use cases for DMI, which has fields 
>> for the processor/socket as well as the machine vendor.
> 
> I did consider DMI, but I want something more generic, i.e. could cover 
> embedded/DT systems also.
> 
> And I need to double check if DMI really has the info I require. Last 
> time I checked, it didn't for my dev board, but I know that some fields 
> are simply not filled in.

Well the info is probably there, but that doesn't mean it should be used 
programmatically. As your board shows, its not that reliable. And 
looking at the linked patch I see you mention that.


> 
>>
>> But, quickly looking at the use case, I can't help but think you don't 
>> really want any of the above, or the PPTT id. It seems the mapping 
>> should actually be tied directly to the uncore PMU definition, rather 
>> than a soc/machine/whatever identifier. Which would imply keying off 
>> one of the ACPI object identifiers for the PMU itself.
> 
> So a PMU device (/sys/bus/event_source/devices) does not have a link to 
> the ACPI object identifiers or uncore PMU platform device etc.
> 
> And even if it did, there is no clear link between that ACPI object and 
> the events it supports for that implementation.

Having a direct link isn't ideal either. It seems you do mention the pmu 
naming conventions, which can be controlled based on ACPI object 
identifiers. Something like "uncore_dmc_hsi1" where the appended bits 
could for example vary on _CID+_UID or DT name.

Not sure that is a particularly good suggestion either, but I do think 
its a better idea to tie the mapping to the pmu type/man/version concept 
than the SOC it appears in. The sysfs-bus-event_source-devices-* ABI 
docs are noticeably silent on the format of the pmu name (is that 
somewhere else?).

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Rafael J. Wysocki]

On Tue, Jan 28, 2020 at 7:22 PM John Garry <john.garry@huawei.com> wrote:
>
> On 28/01/2020 17:51, Olof Johansson wrote:
> > Hi,
> >
> > On Tue, Jan 28, 2020 at 3:18 AM John Garry <john.garry@huawei.com> wrote:
> >>
>
> Hi Olof,
>
> >> Add a generic driver for platforms which populate their ACPI PPTT
> >> processor package ID Type Structure according to suggestion in the ACPI
> >> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
> >>
> >> The soc_id is from member LEVEL_2_ID.
> >>
> >> For this, we need to use a whitelist of platforms which are known to
> >> populate the structure as suggested.
> >>
> >> For now, only the vendor and soc_id fields are exposed.
> >>
> >> Signed-off-by: John Garry <john.garry@huawei.com>
> >> ---
> >>   drivers/soc/Makefile       |   1 +
> >>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 103 insertions(+)
> >>   create mode 100644 drivers/soc/acpi_generic.c
> >>
> >> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> >> index 8b49d782a1ab..2a59a30a22cd 100644
> >> --- a/drivers/soc/Makefile
> >> +++ b/drivers/soc/Makefile
> >> @@ -3,6 +3,7 @@
> >>   # Makefile for the Linux Kernel SOC specific device drivers.
> >>   #
> >>
> >> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
> >>   obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> >>   obj-$(CONFIG_SOC_ASPEED)       += aspeed/
> >>   obj-$(CONFIG_ARCH_AT91)                += atmel/
> >
> > Based on everything I've seen so far, this should go under drivers/acpi instead.
>
> soc drivers seem to live in drivers/soc (non-arm32, anyway), so I
> decided on this location. But drivers/acpi would also seem reasonable now.

Any reasons for not putting it into drivers/acpi/pptt.c specifically?

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

>>>>
>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>> ---
>>>>    drivers/soc/Makefile       |   1 +
>>>>    drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 103 insertions(+)
>>>>    create mode 100644 drivers/soc/acpi_generic.c
>>>>
>>>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>>>> index 8b49d782a1ab..2a59a30a22cd 100644
>>>> --- a/drivers/soc/Makefile
>>>> +++ b/drivers/soc/Makefile
>>>> @@ -3,6 +3,7 @@
>>>>    # Makefile for the Linux Kernel SOC specific device drivers.
>>>>    #
>>>>
>>>> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
>>>>    obj-$(CONFIG_ARCH_ACTIONS)     += actions/
>>>>    obj-$(CONFIG_SOC_ASPEED)       += aspeed/
>>>>    obj-$(CONFIG_ARCH_AT91)                += atmel/
>>>
>>> Based on everything I've seen so far, this should go under drivers/acpi instead.
>>
>> soc drivers seem to live in drivers/soc (non-arm32, anyway), so I
>> decided on this location. But drivers/acpi would also seem reasonable now.
> 

Hi Rafael,

> Any reasons for not putting it into drivers/acpi/pptt.c specifically?
> .

I don't think so.

One thing is that the code does a one-time scan of the PPTT to find all 
processor package nodes with ID structures to register the soc devices - 
so we would need some new call from from acpi_init() for that.

Thanks,
John

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Olof Johansson]

On Tue, Jan 28, 2020 at 10:22 AM John Garry <john.garry@huawei.com> wrote:
>
> On 28/01/2020 17:51, Olof Johansson wrote:
> > Hi,
> >
> > On Tue, Jan 28, 2020 at 3:18 AM John Garry <john.garry@huawei.com> wrote:
> >>
>
> Hi Olof,
>
> >> Add a generic driver for platforms which populate their ACPI PPTT
> >> processor package ID Type Structure according to suggestion in the ACPI
> >> spec - see ACPI 6.2, section 5.2.29.3 ID structure Type 2.
> >>
> >> The soc_id is from member LEVEL_2_ID.
> >>
> >> For this, we need to use a whitelist of platforms which are known to
> >> populate the structure as suggested.
> >>
> >> For now, only the vendor and soc_id fields are exposed.
> >>
> >> Signed-off-by: John Garry <john.garry@huawei.com>
> >> ---
> >>   drivers/soc/Makefile       |   1 +
> >>   drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 103 insertions(+)
> >>   create mode 100644 drivers/soc/acpi_generic.c
> >>
> >> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> >> index 8b49d782a1ab..2a59a30a22cd 100644
> >> --- a/drivers/soc/Makefile
> >> +++ b/drivers/soc/Makefile
> >> @@ -3,6 +3,7 @@
> >>   # Makefile for the Linux Kernel SOC specific device drivers.
> >>   #
> >>
> >> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
> >>   obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> >>   obj-$(CONFIG_SOC_ASPEED)       += aspeed/
> >>   obj-$(CONFIG_ARCH_AT91)                += atmel/
> >
> > Based on everything I've seen so far, this should go under drivers/acpi instead.
>
> soc drivers seem to live in drivers/soc (non-arm32, anyway), so I
> decided on this location. But drivers/acpi would also seem reasonable now.

We don't want drivers/soc to be too much of a catch-all -- it is meant
for some of the glue pieces that don't have good homes elsewhere.
Unfortunately, the slope is slippery and we've already gone down it a
bit, but I think we can fairly clearly declare that this kind of
cross-soc material is likely not the right home for it -- especially
when drivers/acpi is a good fit in this case.

> >> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
> >> new file mode 100644
> >> index 000000000000..34a1f5f8e063
> >> --- /dev/null
> >> +++ b/drivers/soc/acpi_generic.c
> >> @@ -0,0 +1,102 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) John Garry, john.garry@huawei.com
> >> + */
> >> +
> >> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/sys_soc.h>
> >> +
> >> +/*
> >> + * Known platforms that fill in PPTT package ID structures according to
> >> + * ACPI spec examples, that being:
> >> + * - Custom driver attribute is in ID Type Structure VENDOR_ID member
> >> + * - SoC id is in ID Type Structure LEVEL_2_ID member
> >> + *    See ACPI SPEC 6.2 Table 5-154 for PPTT ID Type Structure
> >> + */
> >> +static struct acpi_platform_list plat_list[] = {
> >> +       {"HISI  ", "HIP08   ", 0, ACPI_SIG_PPTT, all_versions},
> >> +       { } /* End */
> >> +};
> >
> > As others have said, this will become a mess over time, and will
> > require changes for every new platform. Which, unfortunately, is
> > exactly what ACPI is supposed to provide relief from by making
> > standardized platforms... standardized.
> >
>
> Right, and I think that it can be dropped. As discussed with Sudeep, I
> was concerned how this PPTT ID structure could be interpreted, and had a
> whitelist as a conservative approach.

[...]

> >
> > Hmm, this doesn't look like much of a driver to me. This looks like
> > the export of an attribute to userspace, and should probably be done
> > by ACPI core instead of creating an empty driver for it.
>
> OK, but I'm thinking that having a soc driver can be useful as it is
> common to DT, and so userspace only has to check a single location. And
> the soc driver can also cover multiple-chip systems without have to
> reinvent that code for ACPI core. And it saves adding a new ABI.

While having a single location could be convenient, the actual data
read/written would be different (I'm guessing).

We also already have a supposed standard way of figuring out what SoC
we're on (toplevel compatible for the DT). So no matter what, I think
userspace will need to handle two ways of probing this.


-Olof

Re: [PATCH RFC 0/2] Add basic generic ACPI soc driver

[John Garry]

Hi Jeremy,

>> I did consider DMI, but I want something more generic, i.e. could 
>> cover embedded/DT systems also.
>>
>> And I need to double check if DMI really has the info I require. Last 
>> time I checked, it didn't for my dev board, but I know that some 
>> fields are simply not filled in.
> 
> Well the info is probably there, but that doesn't mean it should be used 
> programmatically. As your board shows, its not that reliable. And 
> looking at the linked patch I see you mention that.

Right, I am trying to stay away from that.

> 
> 
>>
>>>
>>> But, quickly looking at the use case, I can't help but think you 
>>> don't really want any of the above, or the PPTT id. It seems the 
>>> mapping should actually be tied directly to the uncore PMU 
>>> definition, rather than a soc/machine/whatever identifier. Which 
>>> would imply keying off one of the ACPI object identifiers for the PMU 
>>> itself.
>>
>> So a PMU device (/sys/bus/event_source/devices) does not have a link 
>> to the ACPI object identifiers or uncore PMU platform device etc.
>>
>> And even if it did, there is no clear link between that ACPI object 
>> and the events it supports for that implementation.
> 
> Having a direct link isn't ideal either. It seems you do mention the pmu 
> naming conventions, which can be controlled based on ACPI object 
> identifiers.

Not necessarily.

  Something like "uncore_dmc_hsi1" where the appended bits
> could for example vary on _CID+_UID or DT name.

We already do include some naming from ACPI tables in naming (see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c?h=v5.5#n377), 
but this is not good enough. I'll explain below.

> 
> Not sure that is a particularly good suggestion either, but I do think 
> its a better idea to tie the mapping to the pmu type/man/version concept 
> than the SOC it appears in. The sysfs-bus-event_source-devices-* ABI 
> docs are noticeably silent on the format of the pmu name (is that 
> somewhere else?).

I would say that there is a lack of PMU naming convention, which I did 
note in my referenced patchset.

Apart from that, I think that this problem can be better explained with 
the SMMUv3 PMCG example.

So this PMU has support for a number of IMP DEF events. The SMMUv3 PMCG 
has no method to identify the implementation, so we cannot know which 
IMP DEF events are supported for a specific implementation.

The PMCG PMU naming is fixed, and is in the form smmuv3_pmcg_XXXX - so 
we cannot use some special naming. And the XXXX does not tell us 
anything about the implementation to help know the IMP DEF events.

Now the perf tool has support to know which CPU+uncore events are 
supported for a particular CPU through pmu-events feature - see 
tools/perf/pmu-events/README

The perf tool includes a number of per-CPU event tables.

The matching of per-CPU event table the perf tool uses is based on 
knowing the host CPUID - this is easy to retrieve this via some special 
arch-specific CPU reg, etc. So once it knows the CPUID, "perf list" 
command can show all the events for that CPU.

Now we can extend this idea for the PMCG PMU to support the IMP DEF 
events. For this, we add support for a table of "system" PMU events per 
SoC - similar to the CPU tables - containing the PMCG events. We cannot 
use the CPUID to match the event table for SoC, as a CPUID is not always 
specific to a SoC - that's definite for ARM world and definite for 
SMMUv3 PMCG. So then perf tool needs to know some SoC identifier to 
match the per-SoC events table. That's why I want the SoC id in readable 
form in sysfs.

To add a final note on uncore PMUs, for ARM this is bit of grey area. So 
currently we match uncore PMUs on CPUID. However I figure some SoC 
implementer could take, for example, an A72, and add some uncore PMUs. 
As such, we cannot always match on CPUID, so being able to match on a 
SoC identifier would be better also.

Hope it explains.

Thanks,
John

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[Rafael J. Wysocki]

On Tue, Jan 28, 2020 at 8:28 PM John Garry <john.garry@huawei.com> wrote:
>
>
> >>>>
> >>>> Signed-off-by: John Garry <john.garry@huawei.com>
> >>>> ---
> >>>>    drivers/soc/Makefile       |   1 +
> >>>>    drivers/soc/acpi_generic.c | 102 +++++++++++++++++++++++++++++++++++++
> >>>>    2 files changed, 103 insertions(+)
> >>>>    create mode 100644 drivers/soc/acpi_generic.c
> >>>>
> >>>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> >>>> index 8b49d782a1ab..2a59a30a22cd 100644
> >>>> --- a/drivers/soc/Makefile
> >>>> +++ b/drivers/soc/Makefile
> >>>> @@ -3,6 +3,7 @@
> >>>>    # Makefile for the Linux Kernel SOC specific device drivers.
> >>>>    #
> >>>>
> >>>> +obj-$(CONFIG_ACPI_PPTT)                += acpi_generic.o
> >>>>    obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> >>>>    obj-$(CONFIG_SOC_ASPEED)       += aspeed/
> >>>>    obj-$(CONFIG_ARCH_AT91)                += atmel/
> >>>
> >>> Based on everything I've seen so far, this should go under drivers/acpi instead.
> >>
> >> soc drivers seem to live in drivers/soc (non-arm32, anyway), so I
> >> decided on this location. But drivers/acpi would also seem reasonable now.
> >
>
> Hi Rafael,
>
> > Any reasons for not putting it into drivers/acpi/pptt.c specifically?
> > .
>
> I don't think so.
>
> One thing is that the code does a one-time scan of the PPTT to find all
> processor package nodes with ID structures to register the soc devices -
> so we would need some new call from from acpi_init() for that.

Or an extra initcall or similar. [Calls from acpi_init() are basically
for things that need to be strictly ordered in a specific way for some
reason.]

Why would that be a problem?

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

Hi Olof,

>>>
>>> Based on everything I've seen so far, this should go under drivers/acpi instead.
>>
>> soc drivers seem to live in drivers/soc (non-arm32, anyway), so I
>> decided on this location. But drivers/acpi would also seem reasonable now.
> 
> We don't want drivers/soc to be too much of a catch-all -- it is meant
> for some of the glue pieces that don't have good homes elsewhere.
> Unfortunately, the slope is slippery and we've already gone down it a
> bit, but I think we can fairly clearly declare that this kind of
> cross-soc material is likely not the right home for it -- especially
> when drivers/acpi is a good fit in this case.

ok

> 
>>>> diff --git a/drivers/soc/acpi_generic.c b/drivers/soc/acpi_generic.c
>>>> new file mode 100644
>>>> index 000000000000..34a1f5f8e063
>>>> --- /dev/null
>>>> +++ b/drivers/soc/acpi_generic.c
>>>> @@ -0,0 +1,102 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) John Garry, john.garry@huawei.com
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) "SOC ACPI GENERIC: " fmt
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/sys_soc.h>
>>>> +

[...]

>>>
>>> Hmm, this doesn't look like much of a driver to me. This looks like
>>> the export of an attribute to userspace, and should probably be done
>>> by ACPI core instead of creating an empty driver for it.
>>
>> OK, but I'm thinking that having a soc driver can be useful as it is
>> common to DT, and so userspace only has to check a single location. And
>> the soc driver can also cover multiple-chip systems without have to
>> reinvent that code for ACPI core. And it saves adding a new ABI.
> 
> While having a single location could be convenient, the actual data
> read/written would be different (I'm guessing).

Without doubt we would have different data sometimes between ACPI and DT 
FW..

And it is not ideal that the soc_id sysfs file could have different 
contents for the same SoC, depending on ACPI or DT.

> 
> We also already have a supposed standard way of figuring out what SoC
> we're on (toplevel compatible for the DT). 

 From checking some soc drivers, there is a distinction between how 
soc_id and machine is evaluated: machine comes from DT model, which 
looks standard; however soc_id seems to have different methods of 
evaluate, like sometimes reading some system id register (I'm checking 
exynos-chipid.c there).

We're just looking for soc_id. But, as before, it would probably be 
different between ACPI and DT, so not ideal.

So no matter what, I think
> userspace will need to handle two ways of probing this.
> 

That should not be a big problem.

> 

Thanks,
John

Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver

[John Garry]

>>
>>> Any reasons for not putting it into drivers/acpi/pptt.c specifically?
>>> .
>>
>> I don't think so.
>>
>> One thing is that the code does a one-time scan of the PPTT to find all
>> processor package nodes with ID structures to register the soc devices -
>> so we would need some new call from from acpi_init() for that.
> 

Hi Rafael,

 > Or an extra initcall or similar. [Calls from acpi_init() are basically
 > for things that need to be strictly ordered in a specific way for some
 > reason.]>
 > Why would that be a problem?

I don't see a problem if we want to use a soc driver, but that is 
starting to look unlikely.

Alternatively, if we want to create some folder under 
/sys/firmware/acpi, any restriction comes from the folder location.

For a folder like /sys/firmware/acpi/pptt, we need to ensure acpi_kobj 
is initialized; acpi_kobj is set from subsys_init(acpi_init), so 
module_init() for pptt module would suffice.

However if we wanted to make pptt folder a sub-folder from those created 
in acpi_sysfs_init() - then we would need to make that parent folder 
kobj non-static. Again, module_init() would suffice.

Thanks,
John

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[John Garry]

On 28/01/2020 14:54, Sudeep Holla wrote:
>>>> https://lore.kernel.org/linux-arm-kernel/1579876505-113251-6-git-send-email-john.garry@huawei.com/
>>>>
>>> Ah, there's already quite a lot of dependency built for this feature:(
>> Not really. It's only an RFC ATM, and my requirement is a sysfs file to read
>> the SoC id(s) (under ACPI FW). So I would still expect to be able to support
>> this from the SMCCC extension method.
>>

Hi Sudeep,

> As mentioned above, yes the driver would remain almost same for SMCCC
> SOC_ID support too. The main point was: do we need to add support to
> PPTT Type 2 entry when we know there is proposal to deprecate it. I
> would at-least wait to see progress on that until I would add this to
> the kernel.

Not having support in the kernel for a feature which may be officially 
deprecated, i.e public knowledge, in future (maybe 1 year) seems harsh. 
Especially when there is no final released alternative.

So do you know if other OSes use it?

Thanks
John

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[Sudeep Holla]

On Tue, Jan 28, 2020 at 12:34:15PM +0000, Sudeep Holla wrote:
> On Tue, Jan 28, 2020 at 07:14:18PM +0800, John Garry wrote:
> > The ACPI PPTT ID structure (see 6.2 spec, section 5.2.29.3) allows the
> > vendor to provide an identifier (or vendor specific part number) for a
> > particular processor hierarchy node structure. That may be a processor
> > identifier for a processor node, or some chip identifier for a processor
> > package node.
> >
>
> Unfortunately, there were plans to deprecate this in favour of the new
> SOC_ID SMCCC API[1]. I am not sure if you or anyone in your company have
> access to UEFI ASWG mantis where you can look for the ECR for the PPTT
> Type 2 deprecation. I understand it's not ideal, but we need to converge,
> please take a look at both before further discussion.
>
> I personally would not prefer to add the support when I know it is getting
> deprecated. I am not sure on kernel community policy on the same.
>

OK, the details on the proposal to deprecate can be now found in UEFI
bugzilla [1]

--
Regards,
Sudeep

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=2492

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[John Garry]

On 30/01/2020 11:23, Sudeep Holla wrote:
>> I personally would not prefer to add the support when I know it is getting
>> deprecated. I am not sure on kernel community policy on the same.
>>
> OK, the details on the proposal to deprecate can be now found in UEFI
> bugzilla [1]
> 

Wouldn't it be a better approach to propose deprecating the field when 
there is a readily available alternative, i.e. not a spec from a 
different body in beta stage?

To me, this new SMC support will take an appreciable amount of time to 
be implemented in FW by SiPs when actually released. And if it requires 
an ATF upgrade - which I guess it does - then that's a big job.

Thanks,
John

> --
> Regards,
> Sudeep
> 
> [1]https://bugzilla.tianocore.org/show_bug.cgi?id=2492
> .

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[Sudeep Holla]

On Thu, Jan 30, 2020 at 04:12:20PM +0000, John Garry wrote:
> On 30/01/2020 11:23, Sudeep Holla wrote:
> > > I personally would not prefer to add the support when I know it is getting
> > > deprecated. I am not sure on kernel community policy on the same.
> > >
> > OK, the details on the proposal to deprecate can be now found in UEFI
> > bugzilla [1]
> >
>
> Wouldn't it be a better approach to propose deprecating the field when there
> is a readily available alternative, i.e. not a spec from a different body in
> beta stage?
>

Understandable and valid concerns. It would be helpful if you raise it in
the UEFI bugzilla. Your concerns will get lost if you just raise here.

> To me, this new SMC support will take an appreciable amount of time to be
> implemented in FW by SiPs when actually released. And if it requires an ATF
> upgrade - which I guess it does - then that's a big job.
>

Again I do understand, please raise it with the SMCCC specification contact
as listed in the link I shared.

--
Regards,
Sudeep

Re: [PATCH RFC 1/2] ACPI/PPTT: Add acpi_pptt_get_package_info() API

[John Garry]

On 30/01/2020 17:41, Sudeep Holla wrote:
> On Thu, Jan 30, 2020 at 04:12:20PM +0000, John Garry wrote:
>> On 30/01/2020 11:23, Sudeep Holla wrote:
>>>> I personally would not prefer to add the support when I know it is getting
>>>> deprecated. I am not sure on kernel community policy on the same.
>>>>
>>> OK, the details on the proposal to deprecate can be now found in UEFI
>>> bugzilla [1]
>>>
>>
>> Wouldn't it be a better approach to propose deprecating the field when there
>> is a readily available alternative, i.e. not a spec from a different body in
>> beta stage?
>>
> 
> Understandable and valid concerns. It would be helpful if you raise it in
> the UEFI bugzilla. Your concerns will get lost if you just raise here.

ok, thanks. I can do that if and when I can get an account...

> 
>> To me, this new SMC support will take an appreciable amount of time to be
>> implemented in FW by SiPs when actually released. And if it requires an ATF
>> upgrade - which I guess it does - then that's a big job.
>>
> 
> Again I do understand, please raise it with the SMCCC specification contact
> as listed in the link I shared.
> 

I will talk with my FW guys first when they return from CNY.

Cheers,
john