Message-ID: <4fe9f898-63bf-4815-a493-23bdee93481e@quicinc.com>
Date: Tue, 8 Apr 2025 18:53:40 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: <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>,
        <andersson@...nel.org>, <konradybcio@...nel.org>
Subject: Re: [PATCH v3 2/3] i3c: master: Add Qualcomm I3C controller driver

Thanks Krzysztof  !

On 4/4/2025 4:02 PM, Krzysztof Kozlowski wrote:
> On Thu, Apr 03, 2025 at 07:16:43PM GMT, Mukesh Kumar Savaliya wrote:
>> Add support for the Qualcomm I3C controller driver, which implements
>> I3C master functionality as defined in the MIPI Alliance Specification
>> for I3C, Version 1.0.
>>
>> This driver supports master role in SDR mode.
>>
>> Unlike some other I3C master controllers, this implementation
>> does not support In-Band Interrupts (IBI) and Hot-join requests.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
>> ---
>>   drivers/i3c/master/Kconfig         |   13 +
>>   drivers/i3c/master/Makefile        |    1 +
>>   drivers/i3c/master/i3c-qcom-geni.c | 1099 ++++++++++++++++++++++++++++
>>   3 files changed, 1113 insertions(+)
>>   create mode 100644 drivers/i3c/master/i3c-qcom-geni.c
>>
>> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
>> index 77da199c7413..30b768df94c9 100644
>> --- a/drivers/i3c/master/Kconfig
>> +++ b/drivers/i3c/master/Kconfig
>> @@ -44,6 +44,19 @@ config SVC_I3C_MASTER
>>   	help
>>   	  Support for Silvaco I3C Dual-Role Master Controller.
>>   
>> +config I3C_QCOM_GENI
>> +	tristate "Qualcomm Technologies Inc.'s I3C controller driver"
>> +	depends on I3C
>> +	depends on QCOM_GENI_SE
>> +	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 i3c-qcom-geni.
>> +
>>   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..bc11eecd4692 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_I3C_QCOM_GENI)		+= i3c-qcom-geni.o
> 
> Did you just add entry to the end of file? No, don't break ordering.
> That's a standard rule for most subsystems.
> 
Yes, i just kept at the end. Sure, Let me reorder in alphabetical order.
> ...
> 
>> +irqret:
>> +	if (m_stat)
>> +		writel_relaxed(m_stat, gi3c->se.base + SE_GENI_M_IRQ_CLEAR);
>> +
>> +	if (dma) {
>> +		if (dm_tx_st)
>> +			writel_relaxed(dm_tx_st, gi3c->se.base + SE_DMA_TX_IRQ_CLR);
>> +		if (dm_rx_st)
>> +			writel_relaxed(dm_rx_st, gi3c->se.base + SE_DMA_RX_IRQ_CLR);
>> +	}
>> +
>> +	/* if this is err with done-bit not set, handle that through timeout. */
>> +	if (m_stat & M_CMD_DONE_EN || m_stat & M_CMD_ABORT_EN) {
>> +		writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
>> +		complete(&gi3c->done);
>> +	} else if (dm_tx_st & TX_DMA_DONE || dm_rx_st & RX_DMA_DONE	||
>> +		dm_rx_st & RX_RESET_DONE) {
>> +		complete(&gi3c->done);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&gi3c->irq_lock, flags);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int i3c_geni_runtime_get_mutex_lock(struct geni_i3c_dev *gi3c)
>> +{
> 
> You miss sparse/lockdep annotations.
> 
This is called in pair only, but to avoid repeated code in caller 
functions, we have designed this wrapper.
i3c_geni_runtime_get_mutex_lock()
i3c_geni_runtime_put_mutex_unlock().

caller function maintains the parity. e.g. geni_i3c_master_priv_xfers().

Does a comment help here ? Then i can write up to add.

>> +	int ret;
>> +
>> +	mutex_lock(&gi3c->lock);
>> +	reinit_completion(&gi3c->done);
>> +	ret = pm_runtime_get_sync(gi3c->se.dev);
>> +	if (ret < 0) {
>> +		dev_err(gi3c->se.dev, "error turning on SE resources:%d\n", ret);
>> +		pm_runtime_put_noidle(gi3c->se.dev);
>> +		/* Set device in suspended since resume failed */
>> +		pm_runtime_set_suspended(gi3c->se.dev);
>> +		mutex_unlock(&gi3c->lock);
> 
> Either you lock or don't lock, don't mix these up.
> 
Caller is taking care of not calling i3c_geni_runtime_put_mutex_unlock() 
if this failed.
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void i3c_geni_runtime_put_mutex_unlock(struct geni_i3c_dev *gi3c)
>> +{
> 
> Missing annotations.
> 
Shall i add a comment here ?
>> +	pm_runtime_mark_last_busy(gi3c->se.dev);
>> +	pm_runtime_put_autosuspend(gi3c->se.dev);
>> +	mutex_unlock(&gi3c->lock);
>> +}
>> +
>> +static void geni_i3c_abort_xfer(struct geni_i3c_dev *gi3c)
>> +{
>> +	unsigned long time_remaining;
>> +	unsigned long flags;
>> +
>> +	reinit_completion(&gi3c->done);
>> +	spin_lock_irqsave(&gi3c->irq_lock, flags);
>> +	geni_i3c_handle_err(gi3c, GENI_TIMEOUT);
>> +	geni_se_abort_m_cmd(&gi3c->se);
>> +	spin_unlock_irqrestore(&gi3c->irq_lock, flags);
>> +	time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
>> +	if (!time_remaining)
>> +		dev_err(gi3c->se.dev, "Timeout abort_m_cmd\n");
>> +}
> 
> ...
> 
>> +
>> +static int i3c_geni_resources_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	gi3c->se.base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(gi3c->se.base))
>> +		return PTR_ERR(gi3c->se.base);
>> +
>> +	gi3c->se.clk = devm_clk_get(&pdev->dev, "se");
>> +	if (IS_ERR(gi3c->se.clk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(gi3c->se.clk),
>> +							"Unable to get serial engine core clock: %pe\n",
>> +							gi3c->se.clk);
> 
> Totally messed indentation.
> 
yes, corrected.
>> +	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)
>> +		return ret;
>> +
>> +	/* Default source clock (se-clock-frequency) freq is 100Mhz */
>> +	gi3c->clk_src_freq = KHZ(100000);
> 
> And why can't you use clk_get_rate()?
> 
During probe(), we need one time initialization of source clock 
frequencey. HW has no clock set before this.
>> +
>> +	return 0;
>> +}
>> +
>> +static int geni_i3c_probe(struct platform_device *pdev)
>> +{
>> +	u32 proto, tx_depth, fifo_disable;
>> +	struct geni_i3c_dev *gi3c;
> 
> Just store pdev->dev in local dev variable, to simplify everything here.
yes, thats right. But i see other drivers are using same pdev->dev. Is 
it fine ? if really required, will change it.
> 
>> +	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);
>> +
>> +	init_completion(&gi3c->done);
>> +	mutex_init(&gi3c->lock);
>> +	spin_lock_init(&gi3c->irq_lock);
>> +	platform_set_drvdata(pdev, gi3c);
>> +
>> +	ret = i3c_geni_resources_init(gi3c, pdev);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Error Initializing GENI Resources\n");
>> +
>> +	gi3c->irq = platform_get_irq(pdev, 0);
>> +	if (gi3c->irq < 0)
>> +		return dev_err_probe(&pdev->dev, gi3c->irq, "Error getting IRQ number for I3C\n");
>> +
>> +	ret = devm_request_irq(&pdev->dev, gi3c->irq, geni_i3c_irq,
>> +			       IRQF_NO_AUTOEN, dev_name(&pdev->dev), gi3c);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Error registering core IRQ\n");
>> +
>> +	ret = geni_se_resources_on(&gi3c->se);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Error turning resources ON\n");
>> +
>> +	proto = geni_se_read_proto(&gi3c->se);
>> +	if (proto != GENI_SE_I3C) {
>> +		geni_se_resources_off(&gi3c->se);
>> +		return dev_err_probe(&pdev->dev, -ENXIO, "Invalid proto %d\n", proto);
>> +	}
>> +
>> +	fifo_disable = readl_relaxed(gi3c->se.base + GENI_IF_DISABLE_RO);
>> +	if (fifo_disable) {
>> +		geni_se_resources_off(&gi3c->se);
>> +		return dev_err_probe(&pdev->dev, -ENXIO, "GPI DMA mode not supported\n");
>> +	}
>> +
>> +	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_autosuspend_delay(gi3c->se.dev, I3C_AUTO_SUSPEND_DELAY);
>> +	pm_runtime_use_autosuspend(gi3c->se.dev);
>> +	pm_runtime_set_active(gi3c->se.dev);
>> +	pm_runtime_enable(gi3c->se.dev);
>> +
>> +	ret = i3c_master_register(&gi3c->ctrlr, &pdev->dev, &geni_i3c_master_ops, false);
>> +	if (ret) {
>> +		pm_runtime_disable(gi3c->se.dev);
>> +		pm_runtime_set_suspended(gi3c->se.dev);
>> +		pm_runtime_dont_use_autosuspend(gi3c->se.dev);
>> +		return ret;
>> +	}
>> +
>> +	return ret;
> 
> return 0;
> 
Sure, Done.
>> +}
> 
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists