Message-ID: <e5cad9d0-e602-442f-b216-2f655a9526e3@quicinc.com>
Date: Fri, 7 Feb 2025 17:33:56 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <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 2/5/2025 8:16 PM, Krzysztof Kozlowski wrote:
> 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.
> 
Done.
>> +	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?
> 
defined as const, done.
>> +	[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
> 
I have tested on SM8550 MTP only. Below entry in my internal/local DTSI.
Do you mean to say i should add this property in yaml too ?
se-clock-frequency = <100000000>;
> 
>> +	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?
> 
yes, we have taken 2 rounds internally.
Are you saying i should add this into yaml ?  what do you mean by 
hiddern ABI ?
>> +	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
> 
Done
>> +		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.
Sure, Done.
> 
>> +	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.
> 
I see no other drivers using CONFIG_PM around runtime PM. Checked with 
I2C and I3C. hence removing this check and else condition too.
> 
>> +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
> 
Sure, Done.
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists