From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: <shiju.jose@huawei.com>
Cc: <linux-cxl@vger.kernel.org>, <linux-acpi@vger.kernel.org>,
<linux-mm@kvack.org>, <dan.j.williams@intel.com>,
<dave@stgolabs.net>, <dave.jiang@intel.com>,
<alison.schofield@intel.com>, <vishal.l.verma@intel.com>,
<ira.weiny@intel.com>, <linux-edac@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <david@redhat.com>,
<Vilas.Sridharan@amd.com>, <leo.duran@amd.com>,
<Yazen.Ghannam@amd.com>, <rientjes@google.com>,
<jiaqiyan@google.com>, <tony.luck@intel.com>, <Jon.Grimm@amd.com>,
<dave.hansen@linux.intel.com>, <rafael@kernel.org>,
<lenb@kernel.org>, <naoya.horiguchi@nec.com>,
<james.morse@arm.com>, <jthoughton@google.com>,
<somasundaram.a@hpe.com>, <erdemaktas@google.com>,
<pgonda@google.com>, <duenwen@google.com>,
<mike.malvestuto@intel.com>, <gthelen@google.com>,
<wschwartz@amperecomputing.com>, <dferguson@amperecomputing.com>,
<tanxiaofei@huawei.com>, <prime.zeng@hisilicon.com>,
<kangkang.shen@futurewei.com>, <wanghuiqiang@huawei.com>,
<linuxarm@huawei.com>
Subject: Re: [RFC PATCH v6 09/12] ACPI:RASF: Add common library for RASF and RAS2 PCC interfaces
Date: Tue, 20 Feb 2024 13:53:16 +0000 [thread overview]
Message-ID: <20240220135316.00000874@Huawei.com> (raw)
In-Reply-To: <20240215111455.1462-10-shiju.jose@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;
> +};
next prev parent reply other threads:[~2024-02-20 13:53 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 11:14 [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features shiju.jose
2024-02-15 11:14 ` [RFC PATCH v6 01/12] cxl/mbox: Add GET_SUPPORTED_FEATURES mailbox command shiju.jose
2024-02-20 11:02 ` Jonathan Cameron
2024-03-05 23:57 ` fan
2024-02-15 11:14 ` [RFC PATCH v6 02/12] cxl/mbox: Add GET_FEATURE " shiju.jose
2024-02-20 11:14 ` Jonathan Cameron
2024-02-23 12:23 ` Shiju Jose
2024-02-15 11:14 ` [RFC PATCH v6 03/12] cxl/mbox: Add SET_FEATURE " shiju.jose
2024-02-20 11:27 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 04/12] cxl/memscrub: Add CXL device patrol scrub control feature shiju.jose
2024-02-20 11:59 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 05/12] cxl/memscrub: Add CXL device ECS " shiju.jose
2024-02-20 12:17 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 06/12] memory: scrub: Add scrub subsystem driver supports configuring memory scrubs in the system shiju.jose
2024-02-20 12:39 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 07/12] cxl/memscrub: Register CXL device patrol scrub with scrub configure driver shiju.jose
2024-02-20 12:43 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 08/12] cxl/memscrub: Register CXL device ECS " shiju.jose
2024-02-20 13:39 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 09/12] ACPI:RASF: Add common library for RASF and RAS2 PCC interfaces shiju.jose
2024-02-20 13:53 ` Jonathan Cameron [this message]
2024-02-15 11:14 ` [RFC PATCH v6 10/12] ACPICA: ACPI 6.5: Add support for RAS2 table shiju.jose
2024-02-20 13:58 ` Jonathan Cameron
2024-02-15 11:14 ` [RFC PATCH v6 11/12] ACPI:RAS2: Add driver for ACPI RAS2 feature table (RAS2) shiju.jose
2024-02-15 11:14 ` [RFC PATCH v6 12/12] memory: RAS2: Add memory RAS2 driver shiju.jose
2024-02-20 16:12 ` Jonathan Cameron
2024-02-22 0:20 ` [RFC PATCH v6 00/12] cxl: Add support for CXL feature commands, CXL device patrol scrub control and DDR5 ECS control features Dan Williams
2024-02-23 12:16 ` Shiju Jose
2024-02-23 19:42 ` Dan Williams
2024-02-26 10:29 ` Jonathan Cameron
2024-02-29 19:51 ` Dan Williams
2024-03-01 14:41 ` Jonathan Cameron
2024-03-05 0:52 ` Jiaqi Yan
2024-02-29 20:41 ` Tony Luck
2024-03-01 13:19 ` Jonathan Cameron
2024-02-28 22:26 ` John Groves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240220135316.00000874@Huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=Jon.Grimm@amd.com \
--cc=Vilas.Sridharan@amd.com \
--cc=Yazen.Ghannam@amd.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=david@redhat.com \
--cc=dferguson@amperecomputing.com \
--cc=duenwen@google.com \
--cc=erdemaktas@google.com \
--cc=gthelen@google.com \
--cc=ira.weiny@intel.com \
--cc=james.morse@arm.com \
--cc=jiaqiyan@google.com \
--cc=jthoughton@google.com \
--cc=kangkang.shen@futurewei.com \
--cc=lenb@kernel.org \
--cc=leo.duran@amd.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linuxarm@huawei.com \
--cc=mike.malvestuto@intel.com \
--cc=naoya.horiguchi@nec.com \
--cc=pgonda@google.com \
--cc=prime.zeng@hisilicon.com \
--cc=rafael@kernel.org \
--cc=rientjes@google.com \
--cc=shiju.jose@huawei.com \
--cc=somasundaram.a@hpe.com \
--cc=tanxiaofei@huawei.com \
--cc=tony.luck@intel.com \
--cc=vishal.l.verma@intel.com \
--cc=wanghuiqiang@huawei.com \
--cc=wschwartz@amperecomputing.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).