Re: [RFC PATCH v6 09/12] ACPI:RASF: Add common library for RASF and RAS2 PCC interfaces

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 15 Feb 2024 19:14:51 +0800
<shiju.jose@huawei.com> wrote:

> From: A Somasundaram <somasundaram.a@hpe.com>
> 
> The code contains PCC interfaces for RASF and RAS2 table, functions to send
> RASF commands as per ACPI 5.1 and RAS2 commands as per ACPI 6.5 & upwards
> revision.
> 
> References for this implementation,
> ACPI specification 6.5, section 5.2.20 for RASF table, section 5.2.21 for RAS2
> table and chapter 14 for PCC (Platform Communication Channel).
> 
> Driver uses PCC interfaces to communicate to the ACPI HW.
> This code implements PCC interfaces and the functions to send the RASF/RAS2
> commands to be used by OSPM.
> 
> Signed-off-by: A Somasundaram <somasundaram.a@hpe.com>
> Co-developed-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Shiju Jose <shiju.jose@huawei.com>

I looked at this in depth a while back so this time a quicker review. Just some
trivial stuff inline.

> diff --git a/drivers/acpi/rasf_acpi_common.c b/drivers/acpi/rasf_acpi_common.c
> new file mode 100755
> index 000000000000..3ee34f5d12d3
> --- /dev/null
> +++ b/drivers/acpi/rasf_acpi_common.c
> @@ -0,0 +1,272 @@

...

> +
> +#include <linux/export.h>
> +#include <linux/delay.h>
> +#include <linux/ktime.h>
> +#include <linux/platform_device.h>
> +#include <acpi/rasf_acpi.h>
> +#include <acpi/acpixf.h>
> +
> +static int rasf_check_pcc_chan(struct rasf_context *rasf_ctx)
> +{
> +	int ret = -EIO;
> +	struct acpi_rasf_shared_memory  __iomem *generic_comm_base = rasf_ctx->pcc_comm_addr;
> +	ktime_t next_deadline = ktime_add(ktime_get(), rasf_ctx->deadline);
> +
> +	while (!ktime_after(ktime_get(), next_deadline)) {
> +		/*
> +		 * As per ACPI spec, the PCC space wil be initialized by
> +		 * platform and should have set the command completion bit when
> +		 * PCC can be used by OSPM
> +		 */
> +		if (readw_relaxed(&generic_comm_base->status) & RASF_PCC_CMD_COMPLETE) {
> +			ret = 0;

			return 0;

> +			break;
> +		}
> +		/*
> +		 * Reducing the bus traffic in case this loop takes longer than
> +		 * a few retries.
> +		 */
> +		udelay(10);
> +	}
> +
> +	return ret;

return -EIO;

> +}
> +
> +/**
> + * rasf_send_pcc_cmd() - Send RASF command via PCC channel
> + * @rasf_ctx:	pointer to the rasf context structure
> + * @cmd:	command to send
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int rasf_send_pcc_cmd(struct rasf_context *rasf_ctx, u16 cmd)
> +{
> +	int ret = -EIO;

Looks like it's overwritten in all paths where ret is used.

> +	struct acpi_rasf_shared_memory  *generic_comm_base =
> +		(struct acpi_rasf_shared_memory *)rasf_ctx->pcc_comm_addr;
> +	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
> +	static int mpar_count;
> +	unsigned int time_delta;
> +
> +	if (cmd == RASF_PCC_CMD_EXEC) {
> +		ret = rasf_check_pcc_chan(rasf_ctx);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Handle the Minimum Request Turnaround Time(MRTT)
> +	 * "The minimum amount of time that OSPM must wait after the completion
> +	 * of a command before issuing the next command, in microseconds"
> +	 */
> +	if (rasf_ctx->pcc_mrtt) {
> +		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
> +		if (rasf_ctx->pcc_mrtt > time_delta)
> +			udelay(rasf_ctx->pcc_mrtt - time_delta);
> +	}
> +
> +	/*
> +	 * Handle the non-zero Maximum Periodic Access Rate(MPAR)
> +	 * "The maximum number of periodic requests that the subspace channel can
> +	 * support, reported in commands per minute. 0 indicates no limitation."
> +	 *
> +	 * This parameter should be ideally zero or large enough so that it can
> +	 * handle maximum number of requests that all the cores in the system can
> +	 * collectively generate. If it is not, we will follow the spec and just
> +	 * not send the request to the platform after hitting the MPAR limit in
> +	 * any 60s window
> +	 */
> +	if (rasf_ctx->pcc_mpar) {
> +		if (mpar_count == 0) {
> +			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
> +			if (time_delta < 60 * MSEC_PER_SEC) {
> +				pr_debug("PCC cmd not sent due to MPAR limit");
> +				return -EIO;
> +			}
> +			last_mpar_reset = ktime_get();
> +			mpar_count = rasf_ctx->pcc_mpar;
> +		}
> +		mpar_count--;
> +	}
> +
> +	/* Write to the shared comm region. */
> +	writew_relaxed(cmd, &generic_comm_base->command);
> +
> +	/* Flip CMD COMPLETE bit */
> +	writew_relaxed(0, &generic_comm_base->status);
> +
> +	/* Ring doorbell */
> +	ret = mbox_send_message(rasf_ctx->pcc_channel, &cmd);
> +	if (ret < 0) {
> +		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
> +				cmd, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * For READs we need to ensure the cmd completed to ensure
> +	 * the ensuing read()s can proceed. For WRITEs we dont care
> +	 * because the actual write()s are done before coming here
> +	 * and the next READ or WRITE will check if the channel
> +	 * is busy/free at the entry of this call.
> +	 *
> +	 * If Minimum Request Turnaround Time is non-zero, we need
> +	 * to record the completion time of both READ and WRITE
> +	 * command for proper handling of MRTT, so we need to check
> +	 * for pcc_mrtt in addition to CMD_READ
> +	 */
> +	if (cmd == RASF_PCC_CMD_EXEC || rasf_ctx->pcc_mrtt) {
> +		ret = rasf_check_pcc_chan(rasf_ctx);
> +		if (rasf_ctx->pcc_mrtt)
> +			last_cmd_cmpl_time = ktime_get();
> +	}
> +
> +	if (rasf_ctx->pcc_channel->mbox->txdone_irq)
> +		mbox_chan_txdone(rasf_ctx->pcc_channel, ret);
> +	else
> +		mbox_client_txdone(rasf_ctx->pcc_channel, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(rasf_send_pcc_cmd);
> +
> +/**
> + * rasf_register_pcc_channel() - Register PCC channel
> + * @rasf_ctx:	pointer to the rasf context structure
> + *
> + * Returns: 0 on success, an error otherwise
> + */
> +int rasf_register_pcc_channel(struct rasf_context *rasf_ctx)
> +{
> +	u64 usecs_lat;
> +	unsigned int len;
> +	struct pcc_mbox_chan *pcc_chan;
> +	struct mbox_client *rasf_mbox_cl;
> +	struct acpi_pcct_hw_reduced *rasf_ss;
> +
> +	rasf_mbox_cl = &rasf_ctx->mbox_client;
> +	if (!rasf_mbox_cl || rasf_ctx->pcc_subspace_idx < 0)
> +		return -EINVAL;
> +
> +	pcc_chan = pcc_mbox_request_channel(rasf_mbox_cl,
> +				rasf_ctx->pcc_subspace_idx);
> +
> +	if (IS_ERR(pcc_chan)) {
> +		pr_err("Failed to find PCC channel for subspace %d\n",
> +		       rasf_ctx->pcc_subspace_idx);
> +		return -ENODEV;
> +	}
> +	rasf_ctx->pcc_chan = pcc_chan;

If you are storing the chan, why do we need to sparately
store mchan?

> +	rasf_ctx->pcc_channel = pcc_chan->mchan;
> +	/*
> +	 * The PCC mailbox controller driver should
> +	 * have parsed the PCCT (global table of all
> +	 * PCC channels) and stored pointers to the
> +	 * subspace communication region in con_priv.
> +	 */
> +	rasf_ss = rasf_ctx->pcc_channel->con_priv;
> +
> +	if (!rasf_ss) {
> +		pr_err("No PCC subspace found for RASF\n");
> +		pcc_mbox_free_channel(rasf_ctx->pcc_chan);
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * This is the shared communication region
> +	 * for the OS and Platform to communicate over.
> +	 */
> +	rasf_ctx->comm_base_addr = rasf_ss->base_address;
> +	len = rasf_ss->length;
> +	pr_debug("PCC subspace for RASF=0x%llx len=%d\n",
> +		  rasf_ctx->comm_base_addr, len);

dev_dbg(rasf_ctx->dev ...

throughout probably better.

> +
> +	/*
> +	 * rasf_ss->latency is just a Nominal value. In reality
> +	 * the remote processor could be much slower to reply.
> +	 * So add an arbitrary amount of wait on top of Nominal.
> +	 */
> +	usecs_lat = RASF_NUM_RETRIES * rasf_ss->latency;
> +	rasf_ctx->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
> +	rasf_ctx->pcc_mrtt = rasf_ss->min_turnaround_time;
> +	rasf_ctx->pcc_mpar = rasf_ss->max_access_rate;
> +	rasf_ctx->pcc_comm_addr = acpi_os_ioremap(rasf_ctx->comm_base_addr, len);
> +	pr_debug("pcc_comm_addr=%p\n", rasf_ctx->pcc_comm_addr);
> +
> +	/* Set flag so that we dont come here for each CPU. */
> +	rasf_ctx->pcc_channel_acquired = true;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rasf_register_pcc_channel);

> +/**
> + * rasf_add_platform_device() - Add a platform device for RASF
> + * @name:	name of the device we're adding
> + * @data:	platform specific data for this platform device
> + * @size:	size of platform specific data
> + *
> + * Returns: pointer to platform device on success, an error otherwise

I wonder if we should just rename this to ras2 and ignore the fact
it came form rasf?

> + */
> +struct platform_device *rasf_add_platform_device(char *name, const void *data,
> +						 size_t size)
> +{
> +	int ret;
> +	struct platform_device *pdev;
> +
> +	pdev = platform_device_alloc(name, PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return NULL;
> +
> +	ret = platform_device_add_data(pdev, data, size);
> +	if (ret)
> +		goto dev_put;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto dev_put;
> +
> +	return pdev;
> +
> +dev_put:
> +	platform_device_put(pdev);
> +
> +	return NULL;
Could return an error pointer to provide more info from ret?

> +}
> diff --git a/include/acpi/rasf_acpi.h b/include/acpi/rasf_acpi.h
> new file mode 100644
> index 000000000000..aa4f935b28cf
> --- /dev/null
> +++ b/include/acpi/rasf_acpi.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * RASF driver header file
> + *
> + * (C) Copyright 2014, 2015 Hewlett-Packard Enterprises
> + *
> + * Copyright (c) 2023 HiSilicon Limited
> + */
> +
> +#ifndef _RASF_ACPI_H
> +#define _RASF_ACPI_H
> +
> +#include <linux/acpi.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/types.h>
> +#include <acpi/pcc.h>
> +
> +#define RASF_PCC_CMD_COMPLETE 1
> +
> +/* RASF specific PCC commands */
> +#define RASF_PCC_CMD_EXEC 0x01
> +
> +#define RASF_FAILURE 0
> +#define RASF_SUCCESS 1
> +
> +/*
> + * Arbitrary Retries for PCC commands.

Perhaps a comment on why PCC retry might be needed?

> + */
> +#define RASF_NUM_RETRIES 600
> +
> +/*
> + * Data structures for PCC communication and RASF table
> + */
> +struct rasf_context {
> +	struct device *dev;
> +	int id;
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *pcc_channel;
> +	struct pcc_mbox_chan *pcc_chan;
> +	void __iomem *pcc_comm_addr;
> +	u64 comm_base_addr;
> +	int pcc_subspace_idx;
> +	bool pcc_channel_acquired;
> +	ktime_t deadline;
Perhaps move all the pcc channel specific stuff to a named struct

	struct {
		unsigned int mpar;
		unsigned int mrtt;
	} pcc;

> +	unsigned int pcc_mpar;
> +	unsigned int pcc_mrtt;
> +	spinlock_t spinlock; /* Lock to provide mutually exclusive access to PCC channel */

Move comment to line above. Saves on long line without loss of readability.

> +	struct device *scrub_dev;
> +	const struct rasf_hw_scrub_ops *ops;
> +};