* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 11:14 ` [PATCH RFC 2/2] soc: Add a basic ACPI generic driver John Garry
@ 2020-01-28 11:56 ` Greg KH
2020-01-28 13:33 ` John Garry
2020-01-28 12:50 ` Arnd Bergmann
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Greg KH @ 2020-01-28 11:56 UTC (permalink / raw)
To: John Garry
Cc: rjw, lenb, jeremy.linton, arnd, olof, linux-kernel, linux-acpi,
guohanjun
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 11:56 ` Greg KH
@ 2020-01-28 13:33 ` John Garry
0 siblings, 0 replies; 30+ messages in thread
From: John Garry @ 2020-01-28 13:33 UTC (permalink / raw)
To: Greg KH
Cc: rjw, lenb, jeremy.linton, arnd, olof, linux-kernel, linux-acpi,
guohanjun
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 11:14 ` [PATCH RFC 2/2] soc: Add a basic ACPI generic driver John Garry
2020-01-28 11:56 ` Greg KH
@ 2020-01-28 12:50 ` Arnd Bergmann
2020-01-28 14:46 ` John Garry
2020-01-28 15:20 ` Sudeep Holla
2020-01-28 17:51 ` Olof Johansson
3 siblings, 1 reply; 30+ messages in thread
From: Arnd Bergmann @ 2020-01-28 12:50 UTC (permalink / raw)
To: John Garry
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Olof Johansson,
linux-kernel, ACPI Devel Maling List, Hanjun Guo, gregkh
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 12:50 ` Arnd Bergmann
@ 2020-01-28 14:46 ` John Garry
0 siblings, 0 replies; 30+ messages in thread
From: John Garry @ 2020-01-28 14:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Olof Johansson,
linux-kernel, ACPI Devel Maling List, Hanjun Guo, gregkh
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 11:14 ` [PATCH RFC 2/2] soc: Add a basic ACPI generic driver John Garry
2020-01-28 11:56 ` Greg KH
2020-01-28 12:50 ` Arnd Bergmann
@ 2020-01-28 15:20 ` Sudeep Holla
2020-01-28 15:59 ` John Garry
2020-01-28 17:51 ` Olof Johansson
3 siblings, 1 reply; 30+ messages in thread
From: Sudeep Holla @ 2020-01-28 15:20 UTC (permalink / raw)
To: John Garry
Cc: rjw, lenb, jeremy.linton, arnd, olof, linux-kernel, linux-acpi,
guohanjun, gregkh, 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 15:20 ` Sudeep Holla
@ 2020-01-28 15:59 ` John Garry
2020-01-28 16:17 ` Sudeep Holla
0 siblings, 1 reply; 30+ messages in thread
From: John Garry @ 2020-01-28 15:59 UTC (permalink / raw)
To: Sudeep Holla
Cc: rjw, lenb, jeremy.linton, arnd, olof, linux-kernel, linux-acpi,
guohanjun, gregkh
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 15:59 ` John Garry
@ 2020-01-28 16:17 ` Sudeep Holla
0 siblings, 0 replies; 30+ messages in thread
From: Sudeep Holla @ 2020-01-28 16:17 UTC (permalink / raw)
To: John Garry
Cc: rjw, lenb, jeremy.linton, arnd, olof, linux-kernel, linux-acpi,
guohanjun, gregkh, 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 11:14 ` [PATCH RFC 2/2] soc: Add a basic ACPI generic driver John Garry
` (2 preceding siblings ...)
2020-01-28 15:20 ` Sudeep Holla
@ 2020-01-28 17:51 ` Olof Johansson
2020-01-28 18:22 ` John Garry
3 siblings, 1 reply; 30+ messages in thread
From: Olof Johansson @ 2020-01-28 17:51 UTC (permalink / raw)
To: John Garry
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Arnd Bergmann,
Linux Kernel Mailing List, linux-acpi, Hanjun Guo,
Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 17:51 ` Olof Johansson
@ 2020-01-28 18:22 ` John Garry
2020-01-28 19:11 ` Rafael J. Wysocki
2020-01-28 20:06 ` Olof Johansson
0 siblings, 2 replies; 30+ messages in thread
From: John Garry @ 2020-01-28 18:22 UTC (permalink / raw)
To: Olof Johansson
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Arnd Bergmann,
Linux Kernel Mailing List, linux-acpi, Hanjun Guo,
Greg Kroah-Hartman
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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 18:22 ` John Garry
@ 2020-01-28 19:11 ` Rafael J. Wysocki
2020-01-28 19:28 ` John Garry
2020-01-28 20:06 ` Olof Johansson
1 sibling, 1 reply; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-01-28 19:11 UTC (permalink / raw)
To: John Garry
Cc: Olof Johansson, Rafael J. Wysocki, Len Brown, Jeremy Linton,
Arnd Bergmann, Linux Kernel Mailing List, ACPI Devel Maling List,
Hanjun Guo, Greg Kroah-Hartman
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 19:11 ` Rafael J. Wysocki
@ 2020-01-28 19:28 ` John Garry
2020-01-28 22:30 ` Rafael J. Wysocki
0 siblings, 1 reply; 30+ messages in thread
From: John Garry @ 2020-01-28 19:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Olof Johansson, Rafael J. Wysocki, Len Brown, Jeremy Linton,
Arnd Bergmann, Linux Kernel Mailing List, ACPI Devel Maling List,
Hanjun Guo, Greg Kroah-Hartman
>>>>
>>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 19:28 ` John Garry
@ 2020-01-28 22:30 ` Rafael J. Wysocki
2020-01-29 10:27 ` John Garry
0 siblings, 1 reply; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-01-28 22:30 UTC (permalink / raw)
To: John Garry
Cc: Rafael J. Wysocki, Olof Johansson, Rafael J. Wysocki, Len Brown,
Jeremy Linton, Arnd Bergmann, Linux Kernel Mailing List,
ACPI Devel Maling List, Hanjun Guo, Greg Kroah-Hartman
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?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 22:30 ` Rafael J. Wysocki
@ 2020-01-29 10:27 ` John Garry
0 siblings, 0 replies; 30+ messages in thread
From: John Garry @ 2020-01-29 10:27 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Olof Johansson, Rafael J. Wysocki, Len Brown, Jeremy Linton,
Arnd Bergmann, Linux Kernel Mailing List, ACPI Devel Maling List,
Hanjun Guo, Greg Kroah-Hartman
>>
>>> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 18:22 ` John Garry
2020-01-28 19:11 ` Rafael J. Wysocki
@ 2020-01-28 20:06 ` Olof Johansson
2020-01-29 9:58 ` John Garry
1 sibling, 1 reply; 30+ messages in thread
From: Olof Johansson @ 2020-01-28 20:06 UTC (permalink / raw)
To: John Garry
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Arnd Bergmann,
Linux Kernel Mailing List, linux-acpi, Hanjun Guo,
Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH RFC 2/2] soc: Add a basic ACPI generic driver
2020-01-28 20:06 ` Olof Johansson
@ 2020-01-29 9:58 ` John Garry
0 siblings, 0 replies; 30+ messages in thread
From: John Garry @ 2020-01-29 9:58 UTC (permalink / raw)
To: Olof Johansson
Cc: Rafael J. Wysocki, Len Brown, jeremy.linton, Arnd Bergmann,
Linux Kernel Mailing List, linux-acpi, Guohanjun (Hanjun Guo),
Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 30+ messages in thread