Re: [RFC PATCH 3/4] ultrasoc: Add ultrasoc AXI Communicator driver

On Tue, Jun 15, 2021 at 05:34:43PM +0800, Qi Liu wrote:

This patch adds driver for ultrasoc AXI Communicator. It includes a platform driver to probe AXI Communicator device, a set of operations to access the service data, and a service work entry which will be called by the standard communicator service.

Signed-off-by: Jonathan Zhou jonathan.zhouwen@huawei.com Signed-off-by: Qi Liu liuqi115@huawei.com

---

drivers/hwtracing/ultrasoc/Kconfig | 9 + drivers/hwtracing/ultrasoc/Makefile | 3 + drivers/hwtracing/ultrasoc/ultrasoc-axi-com.c | 334 ++++++++++++++++++++++++++ drivers/hwtracing/ultrasoc/ultrasoc-axi-com.h | 66 +++++ 4 files changed, 412 insertions(+) create mode 100644 drivers/hwtracing/ultrasoc/ultrasoc-axi-com.c create mode 100644 drivers/hwtracing/ultrasoc/ultrasoc-axi-com.h

diff --git a/drivers/hwtracing/ultrasoc/Kconfig b/drivers/hwtracing/ultrasoc/Kconfig index 90a3934..77429f3 100644 --- a/drivers/hwtracing/ultrasoc/Kconfig +++ b/drivers/hwtracing/ultrasoc/Kconfig @@ -13,4 +13,13 @@ menuconfig ULTRASOC specification and configure the right series of components when a trace source gets enabled. +if ULTRASOC +config ULTRASOC_AXI_COM

• tristate "Ultrasoc AXI communicator drivers"
• help

•  This config enables support for Ultrasoc AXI Bus Communicator
  

•  drivers. The AXI Communicator has upstream and downstream channels,
  

•  the upstream channel is used to transmit user configuration, and
  

•  downstream channel to carry response and trace data to the users.
  

endif diff --git a/drivers/hwtracing/ultrasoc/Makefile b/drivers/hwtracing/ultrasoc/Makefile index a747171..54711a7b 100644 --- a/drivers/hwtracing/ultrasoc/Makefile +++ b/drivers/hwtracing/ultrasoc/Makefile @@ -5,3 +5,6 @@ obj-$(CONFIG_ULTRASOC) += ultrasoc-drv.o ultrasoc-drv-objs := ultrasoc.o

+obj-$(CONFIG_ULTRASOC_AXI_COM) += ultrasoc-axi-com-drv.o +ultrasoc-axi-com-drv-objs := ultrasoc-axi-com.o diff --git a/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.c b/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.c new file mode 100644 index 0000000..af153dd --- /dev/null +++ b/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.c @@ -0,0 +1,334 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright (C) 2021 Hisilicon Limited Permission is hereby granted, free of
• • charge, to any person obtaining a copy of this software and associated
• • documentation files (the "Software"), to deal in the Software without
• • restriction, including without limitation the rights to use, copy, modify,
• • merge, publish, distribute, sublicense, and/or sell copies of the Software,
• • and to permit persons to whom the Software is furnished to do so, subject
• • to the following conditions:
• • The above copyright notice and this permission notice shall be included in
• • all copies or substantial portions of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
• • AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
• • IN THE SOFTWARE.
• • Code herein communicates with and accesses proprietary hardware which is
• • licensed intellectual property (IP) belonging to Siemens Digital Industries
• • Software Ltd.
• • Siemens Digital Industries Software Ltd. asserts and reserves all rights to
• • their intellectual property. This paragraph may not be removed or modified
• • in any way without permission from Siemens Digital Industries Software Ltd.
• */

+#include <linux/acpi.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <asm/unaligned.h>

+#include "ultrasoc-axi-com.h"

+static void axi_com_enable_hw(struct axi_com_drv_data *drvdata) +{

• u32 val;
• val = readl(drvdata->base + AXIC_US_CTL);
• val |= AXIC_US_CTL_EN;
• writel(val, drvdata->base + AXIC_US_CTL);
• val = readl(drvdata->base + AXIC_DS_CTL);
• val |= AXIC_DS_CTL_EN;
• writel(val, drvdata->base + AXIC_DS_CTL);

+}

+static void axi_com_disable_hw(struct axi_com_drv_data *drvdata) +{

• u32 val;
• val = readl(drvdata->base + AXIC_US_CTL);
• val &= ~AXIC_US_CTL_EN;
• writel(val, drvdata->base + AXIC_US_CTL);
• val = readl(drvdata->base + AXIC_DS_CTL);
• val &= ~AXIC_DS_CTL_EN;
• writel(val, drvdata->base + AXIC_DS_CTL);

+}

+static inline bool axi_com_us_buf_full(struct axi_com_drv_data *drvdata) +{

• return readl(drvdata->base + AXIC_US_BUF_STS) & BIT(0);

+}

+static inline bool axi_com_ds_buf_full(struct axi_com_drv_data *drvdata) +{

• return readl(drvdata->base + AXIC_DS_BUF_STS) & BIT(0);

+}

+static int axi_com_try_send_msg(struct axi_com_drv_data *drvdata) +{

• struct msg_descp *msg;
• struct list_head *node;
• int index = 0;
• int unsent;
• u32 data;
• if (axi_com_us_buf_full(drvdata)) {

• dev_err_once(drvdata->dev, "No room for upstream buffer.\n");
  

• return US_SERVICE_IDLE;
  

• }
• spin_lock(&drvdata->us_msg_list_lock);
• if (list_empty(&drvdata->us_msg_head)) {

• spin_unlock(&drvdata->us_msg_list_lock);
  

• return US_SERVICE_IDLE;
  

• }
• node = drvdata->us_msg_head.next;
• list_del(node);
• drvdata->us_msg_cur--;
• msg = container_of(node, struct msg_descp, node);
• spin_unlock(&drvdata->us_msg_list_lock);
• unsent = msg->msg_len;
• dev_dbg(drvdata->dev, "Length of send msg: %d.\n", msg->msg_len);
• while (unsent > 0) {

• data = get_unaligned_le32(&msg->msg_buf[index++]);
  

• writel(data, drvdata->base + AXIC_US_DATA);
  

Who reads the data that gets written here?

• unsent -= AXIC_MSG_LEN_PER_SEND;
  

• }
• kfree(msg);
• return US_SERVICE_ONWORK;

+}

+static int axi_com_try_recv_msg(struct axi_com_drv_data *drvdata) +{

• struct msg_descp tmp_msg = {0};
• struct msg_descp *msg;
• bool lost = false;
• u32 index = 0;
• u32 status, entries, data;
• if (!axi_com_ds_buf_full(drvdata))

• return US_SERVICE_IDLE;
  

• msg = kzalloc(sizeof(*msg), GFP_KERNEL);
• if (!msg) {

• /*
  

•  * create local variable tmp_msg to read and clear
  

•  * the downstream message.
  

•  */
  

• msg = &tmp_msg;
  

• lost = true;
  

• }
• do {

• if (index == USMSG_MAX_IDX) {
  

• 	dev_warn(drvdata->dev, "Illegal message.\n");
  

• 	break;
  

• }
  

• data = readl(drvdata->base + AXIC_DS_DATA);
  

Same thing - who writes the data that gets read here?

• put_unaligned_le32(data, &msg->msg_buf[index++]);
  

• status = readl(drvdata->base + AXIC_DS_RD_STS);
  

• entries = status & GENMASK(7, 4);
  

• msg->msg_len += AXIC_MSG_LEN_PER_REC;
  

• } while (entries != 0);
• if (!lost) {

• spin_lock(&drvdata->ds_msg_list_lock);
  

• drvdata->ds_msg_cur++;
  

• drvdata->ds_msg_counter++;
  

• list_add_tail(&msg->node, &drvdata->ds_msg_head);
  

• spin_unlock(&drvdata->ds_msg_list_lock);
  

• }
• return US_SERVICE_ONWORK;

+}

+static int axi_com_work(struct ultrasoc_com *uscom) +{

• struct axi_com_drv_data *drvdata = ultrasoc_com_get_drvdata(uscom);
• int us_ds_flag;
• us_ds_flag = axi_com_try_recv_msg(drvdata);
• us_ds_flag |= axi_com_try_send_msg(drvdata);

Why does this work has to be done in ultrasoc.c when (as far as I can see) it could be done as part of this driver using something like smp_call_function_single()?

• return us_ds_flag;

+}

+static ssize_t axi_com_show_status(struct ultrasoc_com *uscom, char *buf,

• 		   ssize_t wr_size)
  

+{

• struct axi_com_drv_data *drvdata = ultrasoc_com_get_drvdata(uscom);
• wr_size += sysfs_emit_at(buf, wr_size, "%-20s: %d\n",

• 		 "ds msg list num", drvdata->ds_msg_cur);
  

• wr_size += sysfs_emit_at(buf, wr_size, "%-20s: %d\n",

• 		 "us msg list num", drvdata->us_msg_cur);
  

Same comment as smb.

• return wr_size;

+}

+static void axi_com_put_raw_msg(struct ultrasoc_com *uscom, int msg_size,

• 	unsigned long long msg_data)
  

+{

• struct axi_com_drv_data *drvdata = ultrasoc_com_get_drvdata(uscom);
• struct msg_descp *p_msg;
• p_msg = kmalloc(sizeof(*p_msg), GFP_KERNEL);
• if (!p_msg)

• return;
  

• p_msg->msg_len = msg_size;
• put_unaligned_le64(msg_data, &p_msg->msg_buf[0]);
• spin_lock(&drvdata->us_msg_list_lock);
• list_add_tail(&p_msg->node, &drvdata->us_msg_head);
• drvdata->us_msg_cur++;
• spin_unlock(&drvdata->us_msg_list_lock);
• if (uscom->service_status != ULTRASOC_COM_SERVICE_STOPPED)

• wake_up_process(uscom->service);
  

• else

• dev_warn(uscom->dev, "Com service is not running.\n");
  

+}

+static struct uscom_ops axi_com_ops = {

• .com_status = axi_com_show_status,
• .put_raw_msg = axi_com_put_raw_msg,

+};

+/*

• • Config hardwares on the tracing path, using DSM calls to avoid exposing
• • hardware message format.
• */

+static int axi_com_config_inport(struct axi_com_drv_data *drvdata, bool enable) +{

• struct device *dev = drvdata->dev;
• u32 flag = enable ? 1 : 0;
• union acpi_object *obj;
• guid_t guid;
• if (guid_parse("82ae1283-7f6a-4cbe-aa06-53e8fb24db18", &guid)) {

• dev_err(dev, "Get GUID failed.\n");
  

• return -EINVAL;
  

• }
• obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &guid, 0, flag, NULL);
• if (!obj)

• dev_err(dev, "ACPI handle failed!\n");
  

• ACPI_FREE(obj);
• return 0;

+}

+static int axi_com_config_com_descp(struct platform_device *pdev,

• 		    struct axi_com_drv_data *drvdata)
  

+{

• struct device *parent = pdev->dev.parent;
• struct ultrasoc_com_descp com_descp = {0};
• struct device *dev = &pdev->dev;
• struct ultrasoc_com *com;
• com_descp.name = pdev->name;
• com_descp.com_type = ULTRASOC_COM_TYPE_BOTH;

Both what? From this I have to guess there is a dual purpose to this specific type but there is no documentation as to what that can be. This set is difficult to review because of the general lack of documention.

• com_descp.com_dev = dev;
• com_descp.uscom_ops = &axi_com_ops;
• com_descp.com_work = axi_com_work;
• if (device_property_read_u64(dev, "ultrasoc,default_route",

• 		     &com_descp.default_route_msg)) {
  

• dev_err(dev, "Failed to read default_route!\n");
  

• return -EINVAL;
  

• }
• com = ultrasoc_register_com(parent, &com_descp);
• if (IS_ERR(com)) {

• dev_err(dev, "Failed to register to ultrasoc.\n");
  

• return PTR_ERR(com);
  

• }
• /*

• * record the returned com point in drvdata,
  

• * it will be used to unregister the com
  

• * from ultrasoc.
  

• */
  

• drvdata->com = com;
• return 0;

+}

+static int axi_com_probe(struct platform_device *pdev) +{

• struct axi_com_drv_data *drvdata;
• int ret;
• drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
• if (!drvdata)

• return -ENOMEM;
  

• drvdata->base = devm_platform_ioremap_resource(pdev, 0);
• if (IS_ERR(drvdata->base)) {

• dev_err(&pdev->dev, "Failed to ioremap resource.\n");
  

• return PTR_ERR(drvdata->base);
  

• }
• drvdata->dev = &pdev->dev;
• spin_lock_init(&drvdata->ds_msg_list_lock);
• spin_lock_init(&drvdata->us_msg_list_lock);
• INIT_LIST_HEAD(&drvdata->us_msg_head);
• INIT_LIST_HEAD(&drvdata->ds_msg_head);
• axi_com_enable_hw(drvdata);
• ret = axi_com_config_inport(drvdata, true);
• if (ret)

• return ret;
  

• platform_set_drvdata(pdev, drvdata);
• return axi_com_config_com_descp(pdev, drvdata);

+}

+static int axi_com_remove(struct platform_device *pdev) +{

• struct axi_com_drv_data *drvdata = platform_get_drvdata(pdev);
• int ret;
• if (ultrasoc_unregister_com(drvdata->com) == -EBUSY)

• return -EBUSY;
  

• ret = axi_com_config_inport(drvdata, false);
• if (ret)

• return ret;
  

• axi_com_disable_hw(drvdata);
• usmsg_list_realse_all(&drvdata->ds_msg_head);
• usmsg_list_realse_all(&drvdata->us_msg_head);
• return 0;

+}

+static const struct acpi_device_id ultrasoc_axi_com_acpi_match[] = {

• {"HISI03B1", },
• {},

+};

No need for MODULE_DEVICE_TABLE()?

I am very confused as to what this IP does... And I'm even more confused as to why ultrasoc.c is needed at all. As I pointed out in a previous comment there is a lot of work to do on this patchset but there is no point in writing more while questions about the current design choices are pending.

I am done reviewing this set.

Thanks, Mathieu

+static struct platform_driver axi_com_driver = {

• .driver = {

• .name = "ultrasoc,axi-com",
  

• .acpi_match_table = ultrasoc_axi_com_acpi_match,
  

• },
• .probe = axi_com_probe,
• .remove = axi_com_remove,

+}; +module_platform_driver(axi_com_driver);

+MODULE_DESCRIPTION("Ultrasoc AXI COM driver"); +MODULE_LICENSE("Dual MIT/GPL"); +MODULE_AUTHOR("Jonathan Zhou jonathan.zhouwen@huawei.com"); +MODULE_AUTHOR("Qi Liu liuqi115@huawei.com"); diff --git a/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.h b/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.h new file mode 100644 index 0000000..64bcf83 --- /dev/null +++ b/drivers/hwtracing/ultrasoc/ultrasoc-axi-com.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: MIT */ +/*

• • Copyright (C) 2021 Hisilicon Limited Permission is hereby granted, free of
• • charge, to any person obtaining a copy of this software and associated
• • documentation files (the "Software"), to deal in the Software without
• • restriction, including without limitation the rights to use, copy, modify,
• • merge, publish, distribute, sublicense, and/or sell copies of the Software,
• • and to permit persons to whom the Software is furnished to do so, subject
• • to the following conditions:
• • The above copyright notice and this permission notice shall be included in
• • all copies or substantial portions of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
• • AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
• • IN THE SOFTWARE.
• • Code herein communicates with and accesses proprietary hardware which is
• • licensed intellectual property (IP) belonging to Siemens Digital Industries
• • Software Ltd.
• • Siemens Digital Industries Software Ltd. asserts and reserves all rights to
• • their intellectual property. This paragraph may not be removed or modified
• • in any way without permission from Siemens Digital Industries Software Ltd.
• */

+#ifndef ULTRASOC_AXI_COM_H +#define ULTRASOC_AXI_COM_H

+#include "ultrasoc.h"

+#define AXIC_US_CTL 0X0 /* Upstream general control */ +#define AXIC_US_DATA 0XC /* Upstream message data */ +#define AXIC_US_BUF_STS 0X10 /* Upstream buffer status */

+#define AXIC_DS_CTL 0X80 /* Downstream general contral */ +#define AXIC_DS_DATA 0X8C /* Downstream message data */ +#define AXIC_DS_BUF_STS 0X90 /* Downstream buffer status */ +#define AXIC_DS_RD_STS 0X94 /* Downstream read status */

+#define AXIC_MSG_LEN_PER_SEND 4 +#define AXIC_MSG_LEN_PER_REC 4 +#define AXIC_US_CTL_EN 0x1 +#define AXIC_DS_CTL_EN 0x1

+struct axi_com_drv_data {

• void __iomem *base;
• struct device *dev;
• struct ultrasoc_com *com;
• u32 ds_msg_counter;
• u32 us_msg_cur;
• spinlock_t us_msg_list_lock;
• struct list_head us_msg_head;
• u32 ds_msg_cur;
• spinlock_t ds_msg_list_lock;
• struct list_head ds_msg_head;

+};

+#endif

2.7.4