[3/3] arm_scpi: modify to support acpi
Commit Message
arm_scpi: modify to support acpi
Signed-off-by: Wang Honghui <honghui.wang@ucas.com.cn>
---
drivers/firmware/arm_scpi.c | 188 +++++++++++++++++++++---------
drivers/mailbox/phytium_mailbox.c | 44 +++++++
2 files changed, 180 insertions(+), 52 deletions(-)
Comments
On Thu, Nov 24, 2022 at 03:09:31PM +0800, Wang Honghui wrote:
> arm_scpi: modify to support acpi
>
> Signed-off-by: Wang Honghui <honghui.wang@ucas.com.cn>
> @@ -937,55 +952,116 @@ static int scpi_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
> - resource_size_t size;
> - int idx = scpi_drvinfo->num_chans;
> - struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
> - struct mbox_client *cl = &pchan->cl;
> - struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
> -
> - if (!of_match_node(shmem_of_match, shmem))
> - return -ENXIO;
> + /* Wang Honghui modified to support acpi */
> + if (dev->of_node) {
[...]
> + } else {
> + for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
> + u32 size;
> + u32 addr;
> + int idx = scpi_info->num_chans;
> + struct scpi_chan *pchan = scpi_info->channels + idx;
> + struct mbox_client *cl = &pchan->cl;
> + struct fwnode_handle *fn;
> +
> + fn = dev_fwnode(&(pdev->dev));
> + ret = fwnode_property_read_u32(fn, "shmem_start", &addr);
> + if (ret < 0) {
> + dev_err(dev, "failed to get SCPI payload mem resource\n");
> + return ret;
> + }
> +
> + ret = fwnode_property_read_u32(fn, "shmem_size", &size);
> + if (ret < 0) {
> + dev_err(dev, "failed to ioremap SCPI payload\n");
> + return -EADDRNOTAVAIL;
> + }
ACPI has native mechanisms to describe IO resources, so this doesn't look right at all.
Does ARM have ACPI bindings for SCPI? I don't think we should be inventing
vendor-specific bindings for this....
> + if (!ret) {
> + pchan->chan = phytium_mbox_request_channel(cl);
... so this looks very wrong.
Thanks,
Mark.
Hi Wang,
On Thu, Nov 24, 2022 at 03:09:31PM +0800, Wang Honghui wrote:
> arm_scpi: modify to support acpi
>
1. The commit message is pretty useless. I don't get the complete picture
of why you need this change or what you are trying to do in this change.
2. I am unable to see the series as a whole, just have 2 versions of 3/3
in my inbox.
3. This is not the correct way to use SCPI or mailbox in ACPI. You need
to use PCC and PCC OpRegion to achieve what you want. The whole SCPI
protocol details gets abstracted in the firmware(ACPI ASL)
Hope this helps. So in short, NACK for this approach for ACPI.
Thinks!
When boot from uefi on phytium ft2004(arm64) platform, can't show temp & freq of cpu, but well if boot from uboot for same kernel binary file;
So i modified arm_scpi.c and scpi-hwmon.c as patch, and tested ok.
Best Regards!
Wang Honghui
------------------ Original ------------------From: "Mark Rutland"<mark.rutland@arm.com>;Date: Thu, Nov 24, 2022 07:03 PMTo: "Wang Honghui"<honghui.wang@ucas.com.cn>; Cc: "Sudeep Holla"<sudeep.holla@arm.com>; "Cristian Maruss"<cristian.marussi@arm.com>; "Jassi Brar"<jassisinghbrar@gmail.com>; "linux-arm-kernel"<linux-arm-kernel@lists.infradead.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; Subject: Re: [PATCH 3/3] arm_scpi: modify to support acpi On Thu, Nov 24, 2022 at 03:09:31PM +0800, Wang Honghui wrote:> arm_scpi: modify to support acpi> > Signed-off-by: Wang Honghui <honghui.wang@ucas.com.cn>> @@ -937,55 +952,116 @@ static int scpi_probe(struct platform_device *pdev)> if (ret)> return ret;> > - for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {> -resource_size_t size;> -int idx = scpi_drvinfo->num_chans;> -struct scpi_chan *pchan = scpi_drvinfo->channels + idx;> -struct mbox_client *cl = &pchan->cl;> -struct device_node *shmem = of_parse_phandle(np, "shmem", idx);> -> -if (!of_match_node(shmem_of_match, shmem))> - return -ENXIO;> + /* Wang Honghui modified to support acpi */> + if (dev->of_node) {[...]> + } else {> +for (; scpi_info->num_chans < count; scpi_info->num_chans++) {> + u32 size;> + u32 addr;> + int idx = scpi_info->num_chans;> + struct scpi_chan *pchan = scpi_info->channels + idx;> + struct mbox_client *cl = &pchan->cl;> + struct fwnode_handle *fn;> +> + fn = dev_fwnode(&(pdev->dev));> + ret = fwnode_property_read_u32(fn, "shmem_start", &addr);> + if (ret < 0) {> + dev_err(dev, "failed to get SCPI payload mem resource\n");> + return ret;> + }> +> + ret = fwnode_property_read_u32(fn, "shmem_size", &size);> + if (ret < 0) {> + dev_err(dev, "failed to ioremap SCPI payload\n");> + return -EADDRNOTAVAIL;> + }ACPI has native mechanisms to describe IO resources, so this doesn't look right at all.Does ARM have ACPI bindings for SCPI? I don't think we should be inventingvendor-specific bindings for this....> + if (!ret) {> + pchan->chan = phytium_mbox_request_channel(cl);... so this looks very wrong.Thanks,Mark.
On Thu, Nov 24, 2022 at 08:23:58PM +0800, 王洪辉 wrote:
> When boot from uefi on phytium ft2004(arm64) platform, can't show temp &
> freq of cpu, but well if boot from uboot for same kernel binary file.
Yes the same binary must work if you use right ACPI methods/AML/tables.
Everything needed is there in the kernel, you need to write correct ACPI
firmware/tables.
> So i modified arm_scpi.c and scpi-hwmon.c as patch, and tested ok.
>
Yes tested to be OK, but definitely not acceptable. Please look at the
ACPI spec and improve your firmware to use what you need here. I have
given all the pointers now.
@@ -34,6 +34,7 @@
#include <linux/slab.h>
#include <linux/sort.h>
#include <linux/spinlock.h>
+#include <linux/acpi.h>
#define CMD_ID_MASK GENMASK(6, 0)
#define CMD_TOKEN_ID_MASK GENMASK(15, 8)
@@ -907,6 +908,8 @@ static const struct of_device_id shmem_of_match[] __maybe_unused = {
{ }
};
+extern struct mbox_chan *phytium_mbox_request_channel(struct mbox_client *cl);
+
static int scpi_probe(struct platform_device *pdev)
{
int count, idx, ret;
@@ -914,18 +917,30 @@ static int scpi_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *np = dev->of_node;
struct scpi_drvinfo *scpi_drvinfo;
+ const struct acpi_device_id *match;
scpi_drvinfo = devm_kzalloc(dev, sizeof(*scpi_drvinfo), GFP_KERNEL);
if (!scpi_drvinfo)
return -ENOMEM;
- if (of_match_device(legacy_scpi_of_match, &pdev->dev))
- scpi_drvinfo->is_legacy = true;
+ /* Wang Honghui modified to support acpi */
+ if (dev->of_node) {
+ if (of_match_device(legacy_scpi_of_match, &pdev->dev))
+ scpi_drvinfo->is_legacy = true;
- count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
- if (count < 0) {
- dev_err(dev, "no mboxes property in '%pOF'\n", np);
- return -ENODEV;
+ count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
+ if (count < 0) {
+ dev_err(dev, "no mboxes property in '%pOF'\n", np);
+ return -ENODEV;
+ }
+ } else {
+ match = acpi_match_device(dev->driver->acpi_match_table, dev);
+ if (!match) {
+ dev_err(dev, "scpi: Error ACPI match data is missing\n");
+ return -ENODEV;
+ }
+
+ count = 1;
}
scpi_drvinfo->channels =
@@ -937,55 +952,116 @@ static int scpi_probe(struct platform_device *pdev)
if (ret)
return ret;
- for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
- resource_size_t size;
- int idx = scpi_drvinfo->num_chans;
- struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
- struct mbox_client *cl = &pchan->cl;
- struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
-
- if (!of_match_node(shmem_of_match, shmem))
- return -ENXIO;
+ /* Wang Honghui modified to support acpi */
+ if (dev->of_node) {
+ for (; scpi_drvinfo->num_chans < count; scpi_drvinfo->num_chans++) {
+ resource_size_t size;
+ int idx = scpi_drvinfo->num_chans;
+ struct scpi_chan *pchan = scpi_drvinfo->channels + idx;
+ struct mbox_client *cl = &pchan->cl;
+ struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
+
+ if (!of_match_node(shmem_of_match, shmem))
+ return -ENXIO;
+
+ ret = of_address_to_resource(shmem, 0, &res);
+ of_node_put(shmem);
+ if (ret) {
+ dev_err(dev, "failed to get SCPI payload mem resource\n");
+ return ret;
+ }
- ret = of_address_to_resource(shmem, 0, &res);
- of_node_put(shmem);
- if (ret) {
- dev_err(dev, "failed to get SCPI payload mem resource\n");
+ size = resource_size(&res);
+ pchan->rx_payload = devm_ioremap(dev, res.start, size);
+ if (!pchan->rx_payload) {
+ dev_err(dev, "failed to ioremap SCPI payload\n");
+ return -EADDRNOTAVAIL;
+ }
+ pchan->tx_payload = pchan->rx_payload + (size >> 1);
+
+ cl->dev = dev;
+ cl->rx_callback = scpi_handle_remote_msg;
+ cl->tx_prepare = scpi_tx_prepare;
+ cl->tx_block = true;
+ cl->tx_tout = 20;
+ cl->knows_txdone = false; /* controller can't ack */
+
+ INIT_LIST_HEAD(&pchan->rx_pending);
+ INIT_LIST_HEAD(&pchan->xfers_list);
+ spin_lock_init(&pchan->rx_lock);
+ mutex_init(&pchan->xfers_lock);
+
+ ret = scpi_alloc_xfer_list(dev, pchan);
+ if (!ret) {
+ pchan->chan = mbox_request_channel(cl, idx);
+ if (!IS_ERR(pchan->chan))
+ continue;
+ ret = PTR_ERR(pchan->chan);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get channel%d err %d\n",
+ idx, ret);
+ }
return ret;
}
-
- size = resource_size(&res);
- pchan->rx_payload = devm_ioremap(dev, res.start, size);
- if (!pchan->rx_payload) {
- dev_err(dev, "failed to ioremap SCPI payload\n");
- return -EADDRNOTAVAIL;
- }
- pchan->tx_payload = pchan->rx_payload + (size >> 1);
-
- cl->dev = dev;
- cl->rx_callback = scpi_handle_remote_msg;
- cl->tx_prepare = scpi_tx_prepare;
- cl->tx_block = true;
- cl->tx_tout = 20;
- cl->knows_txdone = false; /* controller can't ack */
-
- INIT_LIST_HEAD(&pchan->rx_pending);
- INIT_LIST_HEAD(&pchan->xfers_list);
- spin_lock_init(&pchan->rx_lock);
- mutex_init(&pchan->xfers_lock);
-
- ret = scpi_alloc_xfer_list(dev, pchan);
- if (!ret) {
- pchan->chan = mbox_request_channel(cl, idx);
- if (!IS_ERR(pchan->chan))
- continue;
- ret = PTR_ERR(pchan->chan);
- if (ret != -EPROBE_DEFER)
- dev_err(dev, "failed to get channel%d err %d\n",
- idx, ret);
- }
- return ret;
- }
+ } else {
+ for (; scpi_info->num_chans < count; scpi_info->num_chans++) {
+ u32 size;
+ u32 addr;
+ int idx = scpi_info->num_chans;
+ struct scpi_chan *pchan = scpi_info->channels + idx;
+ struct mbox_client *cl = &pchan->cl;
+ struct fwnode_handle *fn;
+
+ fn = dev_fwnode(&(pdev->dev));
+ ret = fwnode_property_read_u32(fn, "shmem_start", &addr);
+ if (ret < 0) {
+ dev_err(dev, "failed to get SCPI payload mem resource\n");
+ return ret;
+ }
+
+ ret = fwnode_property_read_u32(fn, "shmem_size", &size);
+ if (ret < 0) {
+ dev_err(dev, "failed to ioremap SCPI payload\n");
+ return -EADDRNOTAVAIL;
+ }
+
+ pchan->rx_payload = devm_ioremap(dev, (resource_size_t)addr, (resource_size_t)size);
+
+ if (!pchan->rx_payload) {
+ dev_err(dev, "failed to ioremap SCPI payload\n");
+ return -EADDRNOTAVAIL;
+ }
+ pchan->tx_payload = pchan->rx_payload + (size >> 1);
+
+ cl->dev = dev;
+ cl->rx_callback = scpi_handle_remote_msg;
+ cl->tx_prepare = scpi_tx_prepare;
+ cl->tx_block = true;
+ cl->tx_tout = 20;
+ cl->knows_txdone = false; /* controller can't ack */
+
+ INIT_LIST_HEAD(&pchan->rx_pending);
+ INIT_LIST_HEAD(&pchan->xfers_list);
+ spin_lock_init(&pchan->rx_lock);
+ mutex_init(&pchan->xfers_lock);
+
+ ret = scpi_alloc_xfer_list(dev, pchan);
+
+ if (!ret) {
+ pchan->chan = phytium_mbox_request_channel(cl);
+
+ if (!IS_ERR(pchan->chan))
+ continue;
+
+ ret = PTR_ERR(pchan->chan);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get channel%d err %d\n",
+ idx, ret);
+ }
+
+ return ret;
+ }
+ }
scpi_drvinfo->commands = scpi_std_commands;
@@ -1044,11 +1120,19 @@ static const struct of_device_id scpi_of_match[] = {
MODULE_DEVICE_TABLE(of, scpi_of_match);
+/* Wang Honghui add to support Phytium FT2000/4 CPU for acpi */
+static const struct acpi_device_id scpi_acpi_match[] = {
+ { "PHYT0008", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, scpi_acpi_match);
+
static struct platform_driver scpi_driver = {
.driver = {
.name = "scpi_protocol",
.of_match_table = scpi_of_match,
.dev_groups = versions_groups,
+ .acpi_match_table = ACPI_PTR(scpi_acpi_match),
},
.probe = scpi_probe,
.remove = scpi_remove,
@@ -23,6 +23,10 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
+/* Wang Honghui add */
+#include <linux/mailbox_client.h>
+#include "mailbox.h"
+
#define INTR_STAT 0x0
#define INTR_SET 0x8
#define INTR_CLR 0x10
@@ -31,6 +35,9 @@
#define NR_CHANS 1
+/* Wang Honghui add */
+static struct mbox_chan *phytium_mbox_chan;
+
struct phytium_mbox_link {
unsigned irq;
void __iomem *tx_reg;
@@ -183,6 +190,43 @@ static int phytium_mbox_remove(struct platform_device *pdev)
return 0;
}
+/*
+** Wang Honghui add for scpi to request mbox chan
+*/
+struct mbox_chan *phytium_mbox_request_channel(struct mbox_client *cl)
+{
+ struct device *dev = cl->dev;
+ struct mbox_chan *chan = phytium_mbox_chan;
+ unsigned long flags;
+ int ret;
+
+ if(!chan)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ spin_lock_irqsave(&chan->lock, flags);
+ chan->msg_free = 0;
+ chan->msg_count = 0;
+ chan->active_req = NULL;
+ chan->cl = cl;
+ init_completion(&chan->tx_complete);
+
+ if (chan->txdone_method == TXDONE_BY_POLL && cl->knows_txdone)
+ chan->txdone_method = TXDONE_BY_ACK;
+
+ spin_unlock_irqrestore(&chan->lock, flags);
+
+ if (chan->mbox->ops->startup) {
+ ret = chan->mbox->ops->startup(chan);
+
+ if (ret) {
+
+ dev_err(dev, "Unable to startup the chan (%d)\n", ret);
+ chan = ERR_PTR(ret);
+ }
+ }
+ return chan;
+}
+
static struct platform_driver phytium_mbox_driver = {
.probe = phytium_mbox_probe,
.remove = phytium_mbox_remove,