Date:   Tue, 4 Jan 2022 23:10:23 -0800 (PST)
From:   Ilkka Koskinen <ilkka@os.amperecomputing.com>
To:     Hanjun Guo <guohanjun@huawei.com>
cc:     Ilkka Koskinen <ilkka@os.amperecomputing.com>,
        lorenzo.pieralisi@arm.com, sudeep.holla@arm.com, rafael@kernel.org,
        linux@armlinux.org.uk, lenb@kernel.org, robert.moore@intel.com,
        linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, patches@amperecomputing.com,
        scott@os.amperecomputing.com, darren@os.amperecomputing.com
Subject: Re: [PATCH v3 2/2] ACPI: AGDI: Add driver for Arm Generic Diagnostic
 Dump and Reset device
In-Reply-To: <f0fecf95-0b7d-eeec-94fd-76bea61fdd0f@huawei.com>
Message-ID: <alpine.DEB.2.22.394.2201042306090.2515@ubuntu200401>
References: <20211231033725.21109-1-ilkka@os.amperecomputing.com> <20211231033725.21109-3-ilkka@os.amperecomputing.com> <f0fecf95-0b7d-eeec-94fd-76bea61fdd0f@huawei.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Content-Type: text/plain; charset=US-ASCII; format=flowed
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?YgVSgu00hvc+Hpt8yC3Ej9XLpP/Vz5+5tz13ohPGA+StYKuzOr8adV/qdq8P?=
 =?us-ascii?Q?LE4Uy/JmLi1PzJnsdUevq/8SVltb1UNEKRElOKcsYjlAc83jnpHoHHtcjyXv?=
 =?us-ascii?Q?4Rm3vJ5VY7Lk/nLupmLUfXTkQYcQfXpYMbQ0eQe1MZIsyNnm7FCUceqfxOOW?=
 =?us-ascii?Q?nASUhACiipdxw+6b2O4g3HEqgNgyRfKB7+jAsUiQjytFYOAZ95srzcrcvdtF?=
 =?us-ascii?Q?wkEypw8JuRYiGSnvmoc6JjP9chSbPEBd0bEOpUeagjQ9D9HBZlPZq1vQX8Zk?=
 =?us-ascii?Q?I5Mvk8vLNHzGdnkCrDf12sn4ymDYQjMyTLECe/Nlc88IKjA9qJBuhZDLsd6d?=
 =?us-ascii?Q?F0Q32jwfjjzWu3lXWMBIlilThrRM7OZQWRYGboa1mTnej9o2Nh0i2KQw985G?=
 =?us-ascii?Q?8/uezg5XgF7IXCwQKD4YzdN4ufLJ6QmszhACIvIf9VM/Qes6KvRzb4bgqlMT?=
 =?us-ascii?Q?Ow0Rx69n4Cz9VyC+phizf1LbyGjJFMdq/hElRCjhP8H1cKsaAmlcUC3NceqI?=
 =?us-ascii?Q?SGD9PJyOEQqDpTi9HW9WP1Jh0T0qVaynu1Ayvcirsbif5E1nZxTIp5jvPyHP?=
 =?us-ascii?Q?bi+pPsaZNOFED9wie0kvSwSQGRY9rPdjE9VttJEzz3D4cYJQOCI9Ej+Y1HqI?=
 =?us-ascii?Q?EshdA2e3AlhtcKomvwbf7TaCtsq9tFHBmcVDdZSj58HaeYvTMBi7KF/oVvrg?=
 =?us-ascii?Q?qIjTQT6BPDSlQwaB9D1Gnyb5cBEu104iC7dxIVwNBF7o37XPnmdotmmzJAKW?=
 =?us-ascii?Q?OE0BbFquYCKT0uQl5OyI4ZLYQzzltJxk6byg3N9CxEk9Y4gKGYDxIfgoxMP3?=
 =?us-ascii?Q?rk09EWkKs8E3FO7eUvxrvhBM4ZaioRqF1X9D/KVWj/Eq5ZsxyumXt1pSdgbV?=
 =?us-ascii?Q?CaEfQE6ezgBUttis3oi6kJ+ztNYtCq58k4pm9FhCfmRXopPa/gJEU9Cgionc?=
 =?us-ascii?Q?mkPqW35BiTISvCqXpgYVO/B4RWJQ5TyPFzjE+6t2SFU5owczZE1MWvyqxT+D?=
 =?us-ascii?Q?rMic7SDzEFvtj8CkBD4yihwn3H7xLPRccbDenzUZ1icheaDZvVqLXJW8b4m/?=
 =?us-ascii?Q?Jd5EZvPSXkbgzn0bsQrR3jnTs35JlwtNuAg9Q1GPg26tR4q1zthJzG9KY1BJ?=
 =?us-ascii?Q?XYafqaBKKKP2WVeV27JI6qyxWZ6gcWofNsGrPR90jWKGwt4kL2iG7YZq5/vK?=
 =?us-ascii?Q?H+rMv5snCNfUJdlDncwhCZpkMmnc9Q9jzYUfkE2VZS8yIbQMy31o18bd1zCz?=
 =?us-ascii?Q?By9KqpnJFeoA5hN9ipcpVlWi9QA6p5kyxHpQcAm0ayn8Po9K/h7FRQsUXwqB?=
 =?us-ascii?Q?2Z0UDvuNPU+Dr9T0eW2a36lVwz9uYGfRX4lsJXawutiVAmUkaBJYHcWdGBAe?=
 =?us-ascii?Q?Iy18lXzAApKuBrbdMAYixGp3XWBiuDy+6JiuTOtEg9ygOMgJGfEwC+OPu1hO?=
 =?us-ascii?Q?wkD8lWRG+4nDNQRpfqpd47BtANX0D5z6vMiTO50AHkKHk6hN4dBJylIuLKr7?=
 =?us-ascii?Q?fDcdWsXf2NaQCg5V+cBLwGVuIlNnt10RT1gAKO00xi0aObUowdjdnpuCV+Yv?=
 =?us-ascii?Q?GyclNIefkD2DKWNoBnpCcMiYODmsFcg/xpnteXR+9mYWk6tSGqbvPxWb9L0t?=
 =?us-ascii?Q?f73qLvU+2lSQ9gyDIPDH+q/56XRZGbccTiqNgwtvlM7+IpT3zuH5SR8VqX0q?=
 =?us-ascii?Q?KbRTGoXPz1fr2AyBNLpvETHDkTM8F/b8HvVSgnlVFc75FOFliPEDR7YxSFXl?=
 =?us-ascii?Q?APKikdPPoQ=3D=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d7252833-96b3-48a0-2aef-08d9d01a761d
X-MS-Exchange-CrossTenant-AuthSource: DM5PR0102MB3590.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2022 07:10:32.2110
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: u90CTF8doOGXCdlSUvuMQND2Dn0K/4oMjkYPPJMGnXz8dm3MHt+77tTF1F0k74PCdswnzwpbgcN9WEU/r1FEJdvfsn6CL3UsxLi5cyCdJmg0Cj50F2WC2jp83k/J4rgA
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR01MB4636
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Hi Hanjun,

On Wed, 5 Jan 2022, Hanjun Guo wrote:
> Hi Ilkka,
>
> On 2021/12/31 11:37, Ilkka Koskinen wrote:
>> ACPI for Arm Components 1.1 Platform Design Document v1.1 [0] specifices
>> Arm Generic Diagnostic Device Interface (AGDI). It allows an admin to
>> issue diagnostic dump and reset via an SDEI event or an interrupt.
>> This patch implements SDEI path.
>> 
>> [0] https://developer.arm.com/documentation/den0093/latest/
>> 
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>   drivers/acpi/arm64/Kconfig  |   8 +++
>>   drivers/acpi/arm64/Makefile |   1 +
>>   drivers/acpi/arm64/agdi.c   | 125 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 134 insertions(+)
>>   create mode 100644 drivers/acpi/arm64/agdi.c
>> 
>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>> index 6dba187f4f2e..24869ba5b365 100644
>> --- a/drivers/acpi/arm64/Kconfig
>> +++ b/drivers/acpi/arm64/Kconfig
>> @@ -8,3 +8,11 @@ config ACPI_IORT
>>     config ACPI_GTDT
>>   	bool
>> +
>> +config ACPI_AGDI
>> +	bool "Arm Generic Diagnostic Dump and Reset Device Interface"
>> +	depends on ARM_SDE_INTERFACE
>> +	help
>> +	  Arm Generic Diagnostic Dump and Reset Device Interface (AGDI) is
>> +	  a standard that enables issuing a non-maskable diagnostic dump and
>> +	  reset command.
>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>> index 66acbe77f46e..7b9e4045659d 100644
>> --- a/drivers/acpi/arm64/Makefile
>> +++ b/drivers/acpi/arm64/Makefile
>> @@ -1,4 +1,5 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>> +obj-$(CONFIG_ACPI_AGDI) 	+= agdi.o
>>   obj-$(CONFIG_ACPI_IORT) 	+= iort.o
>>   obj-$(CONFIG_ACPI_GTDT) 	+= gtdt.o
>>   obj-y				+= dma.o
>> diff --git a/drivers/acpi/arm64/agdi.c b/drivers/acpi/arm64/agdi.c
>> new file mode 100644
>> index 000000000000..6525ccbae5c1
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/agdi.c
>> @@ -0,0 +1,125 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * This file implements handling of
>> + * Arm Generic Diagnostic Dump and Reset Interface table (AGDI)
>> + *
>> + * Copyright (c) 2021, Ampere Computing LLC
>> + */
>> +
>> +#define pr_fmt(fmt) "ACPI: AGDI: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/acpi.h>
>> +#include <linux/arm_sdei.h>
>> +#include <linux/io.h>
>
> Please keep the head files in alphabetical order.

I'll fix the include lines

>
>> +
>> +struct agdi_data {
>> +	int sdei_event;
>> +};
>> +
>> +static int agdi_sdei_handler(u32 sdei_event, struct pt_regs *regs, void 
>> *arg)
>> +{
>> +	nmi_panic(regs, "Arm Generic Diagnostic Dump and Reset SDEI event 
>> issued");
>> +	return 0;
>> +}
>> +
>> +static int agdi_sdei_probe(struct platform_device *pdev,
>> +			   struct agdi_data *adata)
>> +{
>> +	int err;
>> +
>> +	err = sdei_event_register(adata->sdei_event, agdi_sdei_handler, 
>> pdev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "Failed to register for SDEI event %d",
>> +			adata->sdei_event);
>> +		return err;
>> +	}
>> +
>> +	err = sdei_event_enable(adata->sdei_event);
>> +	if (err)  {
>> +		sdei_event_unregister(adata->sdei_event);
>> +		dev_err(&pdev->dev, "Failed to enable event %d\n",
>> +			adata->sdei_event);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int agdi_probe(struct platform_device *pdev)
>> +{
>> +	struct agdi_data *adata;
>> +
>> +	adata = dev_get_platdata(&pdev->dev);
>> +	if (!adata)
>> +		return -EINVAL;
>> +
>> +	return agdi_sdei_probe(pdev, adata);
>> +}
>> +
>> +static int agdi_remove(struct platform_device *pdev)
>> +{
>> +	struct agdi_data *adata = platform_get_drvdata(pdev);
>> +
>> +	sdei_event_disable(adata->sdei_event);
>> +	sdei_event_unregister(adata->sdei_event);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver agdi_driver = {
>> +	.driver = {
>> +		.name = "agdi",
>> +	},
>> +	.probe = agdi_probe,
>> +	.remove = agdi_remove,
>> +};
>> +
>> +static int __init agdi_init(void)
>> +{
>> +	int ret;
>> +	acpi_status status;
>> +	struct acpi_table_agdi *agdi_table;
>> +	struct agdi_data pdata;
>> +	struct platform_device *pdev;
>> +
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	status = acpi_get_table(ACPI_SIG_AGDI, 0,
>> +				(struct acpi_table_header **) &agdi_table);
>> +	if (ACPI_FAILURE(status))
>> +		return -ENODEV;
>> +
>> +	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
>> +		pr_warn("Interrupt signaling is not supported");
>> +		ret = -ENODEV;
>> +		goto err_put_table;
>> +	}
>> +
>> +	pdata.sdei_event = agdi_table->sdei_event;
>> +
>> +	pdev = platform_device_register_data(NULL, "agdi", 0, &pdata, 
>> sizeof(pdata));
>> +	if (IS_ERR(pdev)) {
>> +		ret = PTR_ERR(pdev);
>> +		goto err_put_table;
>> +	}
>> +
>> +	ret = platform_driver_register(&agdi_driver);
>> +	if (ret)
>> +		goto err_device_unregister;
>> +
>> +	acpi_put_table((struct acpi_table_header *)agdi_table);
>> +	return 0;
>> +
>> +err_device_unregister:
>> +	platform_device_unregister(pdev);
>> +err_put_table:
>> +	acpi_put_table((struct acpi_table_header *)agdi_table);
>> +	return ret;
>> +}
>> +device_initcall(agdi_init);
>
> The rest are looking good to me.

I'm glad to hear that

> Thanks
> Hanjun
>

Cheers, Ilkka