| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fec85cd8-4c56-4b48-a15f-e7ae08352cc2@kernel.org>
Date: Wed, 5 Feb 2025 15:46:56 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>,
alexandre.belloni@...tlin.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, jarkko.nikula@...ux.intel.com,
linux-i3c@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/3] i3c: master: Add Qualcomm I3C master controller
driver
On 05/02/2025 15:31, Mukesh Kumar Savaliya wrote:
> Add support for the Qualcomm I3C master controller, which implements
> I3C master functionality as defined in the MIPI Alliance Specification
> for I3C, Version 1.0. The driver supports the master role in SDR mode
> only. Unlike some other I3C master controllers, this implementation
> does not support In-Band Interrupts (IBI) and Hot-join requests.
>
> Key features of the Qualcomm I3C master controller driver:
> - Supports I3C master functionality in SDR mode in PIO mode.
> - Does not support IBI and Hot-join requests.
> - Compatible with the MIPI I3C v1.0 specification @
> https://www.mipi.org/mipi-i3c-basic-download-1.0
>
> This addition enhances the I3C subsystem by providing support for
> Qualcomm's hardware supported I3C master controller.
>
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
> ---
> drivers/i3c/master/Kconfig | 12 +
> drivers/i3c/master/Makefile | 1 +
> drivers/i3c/master/qcom-i3c-master.c | 1190 ++++++++++++++++++++++++++
> 3 files changed, 1203 insertions(+)
> create mode 100644 drivers/i3c/master/qcom-i3c-master.c
>
> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
> index 77da199c7413..5370cadfe236 100644
> --- a/drivers/i3c/master/Kconfig
> +++ b/drivers/i3c/master/Kconfig
> @@ -44,6 +44,18 @@ config SVC_I3C_MASTER
> help
> Support for Silvaco I3C Dual-Role Master Controller.
>
> +config QCOM_I3C_MASTER
> + tristate "Qualcomm Technologies Inc.'s I3C Master controller driver"
> + depends on I3C
> + help
> + This driver supports QUPV3 GENI based I3C controller in master
> + mode on the Qualcomm Technologies Inc.s SoCs. If you say yes to
> + this option, support will be included for the built-in I3C interface
> + on the Qualcomm Technologies Inc.s SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called qcom-i3c-master.
> +
> config MIPI_I3C_HCI
> tristate "MIPI I3C Host Controller Interface driver (EXPERIMENTAL)"
> depends on I3C
> diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
> index 3e97960160bc..f6670a63b34f 100644
> --- a/drivers/i3c/master/Makefile
> +++ b/drivers/i3c/master/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o
> obj-$(CONFIG_AST2600_I3C_MASTER) += ast2600-i3c-master.o
> obj-$(CONFIG_SVC_I3C_MASTER) += svc-i3c-master.o
> obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci/
> +obj-$(CONFIG_QCOM_I3C_MASTER) += qcom-i3c-master.o
> diff --git a/drivers/i3c/master/qcom-i3c-master.c b/drivers/i3c/master/qcom-i3c-master.c
> new file mode 100644
> index 000000000000..40566e388386
> --- /dev/null
> +++ b/drivers/i3c/master/qcom-i3c-master.c
> @@ -0,0 +1,1190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i3c/master.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/geni-se.h>
> +
> +#define SE_I3C_SCL_HIGH 0x268
> +#define SE_I3C_TX_TRANS_LEN 0x26C
> +#define SE_I3C_RX_TRANS_LEN 0x270
> +#define SE_I3C_DELAY_COUNTER 0x274
> +#define SE_I2C_SCL_COUNTERS 0x278
> +#define SE_I3C_SCL_CYCLE 0x27C
> +#define SE_GENI_HW_IRQ_EN 0x920
> +#define SE_GENI_HW_IRQ_IGNORE_ON_ACTIVE 0x924
> +#define SE_GENI_HW_IRQ_CMD_PARAM_0 0x930
> +
> +/* SE_GENI_M_CLK_CFG field shifts */
> +#define CLK_DEV_VALUE_SHFT 4
> +#define SER_CLK_EN_SHFT 0
> +
> +/* SE_GENI_HW_IRQ_CMD_PARAM_0 field shifts */
> +#define M_IBI_IRQ_PARAM_7E_SHFT 0
> +#define M_IBI_IRQ_PARAM_STOP_STALL_SHFT 1
> +
> +/* SE_I2C_SCL_COUNTERS field shifts */
> +#define I2C_SCL_HIGH_COUNTER_SHFT 20
> +#define I2C_SCL_LOW_COUNTER_SHFT 10
> +
> +#define SE_I3C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\
> + M_CMD_ABORT_EN | M_GP_IRQ_0_EN | M_GP_IRQ_1_EN | M_GP_IRQ_2_EN | \
> + M_GP_IRQ_3_EN | M_GP_IRQ_4_EN)
> +
> +/* M_CMD OP codes for I2C/I3C */
> +#define I3C_READ_IBI_HW 0
> +#define I2C_WRITE 1
> +#define I2C_READ 2
> +#define I2C_WRITE_READ 3
> +#define I2C_ADDR_ONLY 4
> +#define I3C_INBAND_RESET 5
> +#define I2C_BUS_CLEAR 6
> +#define I2C_STOP_ON_BUS 7
> +#define I3C_HDR_DDR_EXIT 8
> +#define I3C_PRIVATE_WRITE 9
> +#define I3C_PRIVATE_READ 10
> +#define I3C_HDR_DDR_WRITE 11
> +#define I3C_HDR_DDR_READ 12
> +#define I3C_DIRECT_CCC_ADDR_ONLY 13
> +#define I3C_BCAST_CCC_ADDR_ONLY 14
> +#define I3C_READ_IBI 15
> +#define I3C_BCAST_CCC_WRITE 16
> +#define I3C_DIRECT_CCC_WRITE 17
> +#define I3C_DIRECT_CCC_READ 18
> +
> +/* M_CMD params for I3C */
> +#define PRE_CMD_DELAY BIT(0)
> +#define TIMESTAMP_BEFORE BIT(1)
> +#define STOP_STRETCH BIT(2)
> +#define TIMESTAMP_AFTER BIT(3)
> +#define POST_COMMAND_DELAY BIT(4)
> +#define IGNORE_ADD_NACK BIT(6)
> +#define READ_FINISHED_WITH_ACK BIT(7)
> +#define CONTINUOUS_MODE_DAA BIT(8)
> +#define SLV_ADDR_MSK GENMASK(15, 9)
> +#define SLV_ADDR_SHFT 9
> +#define CCC_HDR_CMD_MSK GENMASK(23, 16)
> +#define CCC_HDR_CMD_SHFT 16
> +#define IBI_NACK_TBL_CTRL BIT(24)
> +#define USE_7E BIT(25)
> +#define BYPASS_ADDR_PHASE BIT(26)
> +
> +/* GSI callback error fields */
> +#define GP_IRQ0 0
> +#define GP_IRQ1 1
> +#define GP_IRQ2 2
> +#define GP_IRQ3 3
> +#define GP_IRQ4 4
> +#define GP_IRQ5 5
> +#define DM_I3C_CB_ERR ((BIT(GP_IRQ0) | BIT(GP_IRQ1) | BIT(GP_IRQ2) | \
> + BIT(GP_IRQ3) | BIT(GP_IRQ4) | BIT(GP_IRQ5)) << 5)
> +
> +#define I3C_AUTO_SUSPEND_DELAY 250
> +#define KHZ(freq) (1000 * (freq))
> +#define PACKING_BYTES_PW 4
> +#define XFER_TIMEOUT 250
> +#define DFS_INDEX_MAX 7
> +
> +#define I3C_ADDR_MASK 0x7f
> +
> +enum geni_i3c_err_code {
> + RD_TERM,
> + NACK,
> + CRC_ERR,
> + BUS_PROTO,
> + NACK_7E,
> + NACK_IBI,
> + GENI_OVERRUN,
> + GENI_ILLEGAL_CMD,
> + GENI_ABORT_DONE,
> + GENI_TIMEOUT,
> +};
> +
> +enum i3c_trans_dir {
> + WRITE_TRANSACTION = 0,
> + READ_TRANSACTION = 1
> +};
> +
> +enum i3c_bus_phase {
> + OPEN_DRAIN_MODE = 0,
> + PUSH_PULL_MODE = 1
> +};
> +
> +struct geni_i3c_dev {
> + struct geni_se se;
> + unsigned int tx_wm;
> + int irq;
> + int err;
> + struct i3c_master_controller ctrlr;
> + struct completion done;
> + /* Used inside get_mutex_lock()/unlock() - To manage per device CCC/tranxfer */
Do not say where it is used - we see. Instead, say what is being protected.
> + struct mutex lock;
> + /* Per device protection between process and IRQ context */
> + spinlock_t spinlock;
> + u32 clk_src_freq;
> + u32 dfs_idx;
> + u8 *cur_buf;
> + enum i3c_trans_dir cur_rnw;
> + int cur_len;
> + int cur_idx;
> + unsigned long newaddrslots[(I3C_ADDR_MASK + 1) / BITS_PER_LONG];
> + const struct geni_i3c_clk_fld *clk_fld;
> + const struct geni_i3c_clk_fld *clk_od_fld;
> +};
> +
> +struct geni_i3c_i2c_dev_data {
> + u32 ibi_keeping; /* Plan to save IBI information, keep as dummy for now */
> +};
> +
> +struct geni_i3c_xfer_params {
> + enum geni_se_xfer_mode mode;
> + u32 m_cmd;
> + u32 m_param;
> +};
> +
> +static inline struct geni_i3c_dev *to_geni_i3c_master(struct i3c_master_controller
> + *master)
> +{
> + return container_of(master, struct geni_i3c_dev, ctrlr);
> +}
> +
> +struct geni_i3c_err_log {
> + int err;
> + const char *msg;
> +};
> +
> +static struct geni_i3c_err_log gi3c_log[] = {
Not const? Why?
> + [RD_TERM] = { -EINVAL, "I3C slave early read termination" },
> + [NACK] = { -ENOTCONN, "NACK: slave unresponsive, check power/reset" },
> + [CRC_ERR] = { -EINVAL, "CRC or parity error" },
> + [BUS_PROTO] = { -EPROTO, "Bus proto Error, noisy/unexpected start/stop" },
> + [NACK_7E] = { -EBUSY, "NACK on 7E, unexpected protocol error" },
> + [NACK_IBI] = { -EINVAL, "NACK on IBI" },
> + [GENI_OVERRUN] = { -EIO, "Cmd overrun, check GENI cmd-state machine" },
> + [GENI_ILLEGAL_CMD] = { -EILSEQ,
> + "Illegal cmd, check GENI cmd-state machine" },
> + [GENI_ABORT_DONE] = { -ETIMEDOUT, "Abort after timeout successful" },
> + [GENI_TIMEOUT] = { -ETIMEDOUT, "I3C transaction timed out" },
> +};
> +
...
> +
> +static int i3c_geni_rsrcs_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
> +{
> + int ret;
> + struct resource *res;
> +
> + /* Base register address */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Error getting IO region\n");
> + return -EINVAL;
> + }
> +
> + gi3c->se.base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(gi3c->se.base))
> + return PTR_ERR(gi3c->se.base);
> +
> + gi3c->se.clk = devm_clk_get(&pdev->dev, "se-clk");
> + if (IS_ERR(gi3c->se.clk)) {
> + ret = PTR_ERR(gi3c->se.clk);
> + dev_err(&pdev->dev, "Error getting SE Core clk %d\n", ret);
> + return ret;
> + }
> +
> + ret = device_property_read_u32(&pdev->dev, "se-clock-frequency", &gi3c->clk_src_freq);
You never tested your DTS or this code... Drop
> + if (ret) {
> + dev_info(&pdev->dev, "SE clk freq not specified, default to 100 MHz.\n");
> + gi3c->clk_src_freq = 100000000;
> + }
> +
> + ret = geni_icc_get(&gi3c->se, NULL);
> + if (ret)
> + return ret;
> +
> + /* Set the bus quota to a reasonable value for register access */
> + gi3c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
> + gi3c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
> + ret = geni_icc_set_bw(&gi3c->se);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: icc set bw failed ret:%d\n", __func__, ret);
> + return ret;
> + }
> + dev_dbg(&pdev->dev, "%s: GENI_TO_CORE:%d CPU_TO_GENI:%d\n", __func__,
> + gi3c->se.icc_paths[GENI_TO_CORE].avg_bw, gi3c->se.icc_paths[CPU_TO_GENI].avg_bw);
> +
> + ret = device_property_read_u32(&pdev->dev, "dfs-index", &gi3c->dfs_idx);
The same. You cannot send us hidden ABI.
This code does not look like ready for upstream. Are you sure it was
internally reviewed?
> + if (ret)
> + gi3c->dfs_idx = 0xf;
> +
> + return 0;
> +}
> +
> +static int geni_i3c_probe(struct platform_device *pdev)
> +{
> + u32 proto, tx_depth, fifo_disable;
> + struct geni_i3c_dev *gi3c;
> + int ret;
> +
> + gi3c = devm_kzalloc(&pdev->dev, sizeof(*gi3c), GFP_KERNEL);
> + if (!gi3c)
> + return -ENOMEM;
> +
> + gi3c->se.dev = &pdev->dev;
> + gi3c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +
> + ret = i3c_geni_rsrcs_init(gi3c, pdev);
> + if (ret) {
> + dev_info(&pdev->dev, "Error:%d i3c_geni_rsrcs_init\n", ret);
> + return ret;
> + }
> +
> + gi3c->irq = platform_get_irq(pdev, 0);
> + if (gi3c->irq < 0) {
> + dev_err(&pdev->dev, "Error=%d getting IRQ for i3c-master-geni\n", ret);
> + return gi3c->irq;
> + }
> +
> + init_completion(&gi3c->done);
> + mutex_init(&gi3c->lock);
> + spin_lock_init(&gi3c->spinlock);
> + platform_set_drvdata(pdev, gi3c);
> +
> + ret = devm_request_irq(&pdev->dev, gi3c->irq, geni_i3c_irq,
> + IRQF_TRIGGER_HIGH | IRQF_NO_AUTOEN, dev_name(&pdev->dev), gi3c);
> + if (ret) {
> + dev_err(&pdev->dev, "Request_irq failed:%d: Error:%d\n",
> + gi3c->irq, ret);
> + return ret;
> + }
> +
> + ret = geni_se_resources_on(&gi3c->se);
> + if (ret) {
> + dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
> + return ret;
> + }
> +
> + proto = geni_se_read_proto(&gi3c->se);
> + if (proto != GENI_SE_I3C) {
> + dev_err(&pdev->dev, "Invalid proto %d\n", proto);
> + geni_se_resources_off(&gi3c->se);
> + return -ENXIO;
> + }
> +
> + fifo_disable = readl_relaxed(gi3c->se.base + GENI_IF_DISABLE_RO);
> + if (fifo_disable) {
> + dev_err_probe(&pdev->dev, -ENXIO, "GPI DMA mode not supported\n");
Syntax is always: return dev_err_probe
> + geni_se_resources_off(&gi3c->se);
> + return -ENXIO;
> + }
> +
> + tx_depth = geni_se_get_tx_fifo_depth(&gi3c->se);
> + gi3c->tx_wm = tx_depth - 1;
> + geni_se_init(&gi3c->se, gi3c->tx_wm, tx_depth);
> + geni_se_config_packing(&gi3c->se, BITS_PER_BYTE, PACKING_BYTES_PW, true, true, true);
> + geni_se_resources_off(&gi3c->se);
> + dev_dbg(&pdev->dev, "i3c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +
> + pm_runtime_set_suspended(gi3c->se.dev);
> + pm_runtime_set_autosuspend_delay(gi3c->se.dev, I3C_AUTO_SUSPEND_DELAY);
> + pm_runtime_use_autosuspend(gi3c->se.dev);
> + pm_runtime_enable(gi3c->se.dev);
> +
> + ret = i3c_master_register(&gi3c->ctrlr, &pdev->dev, &geni_i3c_master_ops, false);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&pdev->dev, "Geni I3C probed\n");
No, drop.
> + return ret;
> +}
> +
> +static void geni_i3c_remove(struct platform_device *pdev)
> +{
> + struct geni_i3c_dev *gi3c = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(gi3c->se.dev);
> + i3c_master_unregister(&gi3c->ctrlr);
> +}
> +
> +#ifdef CONFIG_PM
> +static int geni_i3c_runtime_suspend(struct device *dev)
> +{
> + struct geni_i3c_dev *gi3c = dev_get_drvdata(dev);
> +
> + disable_irq(gi3c->irq);
> + geni_se_resources_off(&gi3c->se);
> + return 0;
> +}
> +
> +static int geni_i3c_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct geni_i3c_dev *gi3c = dev_get_drvdata(dev);
> +
> + ret = geni_se_resources_on(&gi3c->se);
> + if (ret)
> + return ret;
> + enable_irq(gi3c->irq);
> + return 0;
> +}
> +#else
Entire else is not necessary. Use proper wrappers for this case. See
other drivers.
> +static int geni_i3c_runtime_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int geni_i3c_runtime_resume(struct device *dev)
> +{
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops geni_i3c_pm_ops = {
> +SET_RUNTIME_PM_OPS(geni_i3c_runtime_suspend, geni_i3c_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id geni_i3c_dt_match[] = {
> + { .compatible = "qcom,geni-i3c" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, geni_i3c_dt_match);
> +
> +static struct platform_driver geni_i3c_master = {
> + .probe = geni_i3c_probe,
> + .remove = geni_i3c_remove,
> + .driver = {
> + .name = "geni_i3c_master",
> + .pm = &geni_i3c_pm_ops,
> + .of_match_table = geni_i3c_dt_match,
> + },
> +};
> +
> +module_platform_driver(geni_i3c_master);
> +
> +MODULE_AUTHOR("Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>");
> +MODULE_DESCRIPTION("Qualcomm I3C Controller Driver for GENI based QUP cores");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:geni_i3c_master");
Drop alias
Best regards,
Krzysztof
Powered by blists - more mailing lists