Re: [PATCH v6 2/3] i3c: master: Add Qualcomm I3C controller driver

[Mukesh Savaliya <mukesh.savaliya@oss.qualcomm.com>]

Thanks Alok for reviewing the patch !

Apologies for the delayed response. I was waiting to verify the changes 
and ensure all the suggested updates were properly incorporated. The 
effort was further delayed due to some slave-side setup issues.

I have now addressed and validated all the suggested changes except one, 
which I have responded to separately below. I shall upload V7 soon.

On 7/1/2025 2:18 PM, ALOK TIWARI wrote:
> 
> 
> On 7/1/2025 12:48 PM, 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 <mukesh.savaliya@oss.qualcomm.com>
>> ---
>>   drivers/i3c/master/Kconfig         |   12 +
>>   drivers/i3c/master/Makefile        |    1 +
>>   drivers/i3c/master/i3c-qcom-geni.c | 1158 ++++++++++++++++++++++++++++
>>   3 files changed, 1171 insertions(+)
>>   create mode 100644 drivers/i3c/master/i3c-qcom-geni.c
>>
>> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig
>> index 7b30db3253af..3e062d4b8f85 100644
>> --- a/drivers/i3c/master/Kconfig
>> +++ b/drivers/i3c/master/Kconfig
>> @@ -41,6 +41,18 @@ 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 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.
> 
> is this .s have any meaning
> 
It's supposed to be "Inc.'s", have corrected. Thanks !
>> +
>> +      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 HAS_IOMEM
>> diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile
>> index 3e97960160bc..0e3ad9d96424 100644
>> --- a/drivers/i3c/master/Makefile
>> +++ b/drivers/i3c/master/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_CDNS_I3C_MASTER)        += i3c-master-cdns.o
>> +obj-$(CONFIG_I3C_QCOM_GENI)        += i3c-qcom-geni.o
>>   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
>> diff --git a/drivers/i3c/master/i3c-qcom-geni.c b/drivers/i3c/master/ 
>> i3c-qcom-geni.c
>> new file mode 100644
>> index 000000000000..c8bb40cdaaf4
>> --- /dev/null
>> +++ b/drivers/i3c/master/i3c-qcom-geni.c
>> @@ -0,0 +1,1158 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + *
>> + * Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
>> + */
>> +
>> +#include <linux/bits.h>
>> +#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>
>> +#include <linux/units.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
>> +
>> +/* HW I3C IBI interrupt enable */
>> +#define M_IBI_IRQ_EN            BIT(0)
>> +
>> +/* M_IBI_IRQ_IGNORE */
>> +#define M_IBI_IRQ_IGNORE        BIT(0)
>> +
>> +/* SE_GENI_M_CLK_CFG field shifts */
>> +#define    CLK_DIV_VALUE_MASK        GENMASK(23, 4)
>> +#define SER_CLK_EN            BIT(0)
>> +
>> +/* SE_GENI_HW_IRQ_CMD_PARAM_0 field bits */
>> +#define M_IBI_IRQ_PARAM_7E        BIT(0)
>> +#define M_IBI_IRQ_PARAM_STOP_STALL    BIT(1)
>> +
>> +/* SE_I2C_SCL_COUNTERS field shifts */
>> +#define I2C_SCL_HIGH_COUNTER_MASK    GENMASK(29, 20)
>> +#define I2C_SCL_LOW_COUNTER_MASK    GENMASK(19, 10)
>> +#define I2C_SCL_CYCLE_COUNTER_MASK    GENMASK(9, 0)
>> +
>> +#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 SLAVE_ADDR_MASK        GENMASK(15, 9)
>> +
>> +#define CCC_HDR_CMD_MSK        GENMASK(23, 16)
>> +#define IBI_NACK_TBL_CTRL    BIT(24)
>> +#define USE_7E            BIT(25)
>> +#define BYPASS_ADDR_PHASE    BIT(26)
>> +
>> +/* GSI callback error fields - DMA_TX_IRQ_STAT */
>> +#define GP_IRQ0            BIT(5)
>> +#define GP_IRQ1            BIT(6)
>> +#define GP_IRQ2            BIT(7)
>> +#define GP_IRQ3            BIT(8)
>> +#define GP_IRQ4            BIT(9)
>> +#define GP_IRQ5            BIT(10)
>> +#define DM_I3C_CB_ERR        GENMASK(10, 5)
>> +
>> +#define I3C_AUTO_SUSPEND_DELAY    250
>> +#define PACKING_BYTES_PER_WORD    4
>> +#define XFER_TIMEOUT        250
>> +#define DFS_INDEX_MAX        7
>> +
>> +#define I3C_ADDR_MASK        I2C_MAX_ADDR
>> +
> [clip]
>> +
>> +static int geni_i3c_master_i2c_xfers(struct i2c_dev_desc *dev, struct 
>> i2c_msg *msgs, int num)
>> +{
>> +    struct i3c_master_controller *m = i2c_dev_get_master(dev);
>> +    struct geni_i3c_dev *gi3c = to_geni_i3c_master(m);
>> +    int i, ret;
>> +
>> +    ret = i3c_geni_runtime_get_mutex_lock(gi3c);
>> +    if (ret)
>> +        return ret;
>> +
>> +    qcom_geni_i3c_conf(gi3c, PUSH_PULL_MODE);
>> +
>> +    for (i = 0; i < num; i++) {
>> +        struct geni_i3c_xfer_params xfer;
>> +
>> +        xfer.m_cmd    = (msgs[i].flags & I2C_M_RD) ? I2C_READ : 
>> I2C_WRITE;
>> +        xfer.m_param  = (i < (num - 1)) ? STOP_STRETCH : 0;
>> +        xfer.m_param |= FIELD_PREP(SLAVE_ADDR_MASK, msgs[i].addr);
>> +        xfer.mode     = msgs[i].len > 32 ? GENI_SE_DMA : GENI_SE_FIFO;
> 
> is this 32 assumptions ,can tx_depth decide this?
> 
It's half of the FIFO size and it's exactly tx_depth. so yes,
we can decide from tx_depth.
Have stored it and kept half of the tx_depth as deciding value.

>> +        if (msgs[i].flags & I2C_M_RD)
>> +            ret = i3c_geni_execute_read_command(gi3c, &xfer, 
>> msgs[i].buf, msgs[i].len);
>> +        else
>> +            ret = i3c_geni_execute_write_command(gi3c, &xfer, 
>> msgs[i].buf, msgs[i].len);
>> +        if (ret)
>> +            break;
>> +    }
>> +
>> +    dev_dbg(gi3c->se.dev, "i2c: txn ret:%d\n", ret);
>> +    i3c_geni_runtime_put_mutex_unlock(gi3c);
>> +
>> +    return ret;
>> +}
>> +
> [clip]
>> +static int geni_i3c_master_bus_init(struct i3c_master_controller *m)
>> +{
>> +    struct geni_i3c_dev *gi3c = to_geni_i3c_master(m);
>> +    struct i3c_bus *bus = i3c_master_get_bus(m);
>> +    struct i3c_device_info info = { };
>> +    int ret;
>> +
>> +    /* Get an address for the master. */
>> +    ret = i3c_master_get_free_addr(m, 0);
>> +    if (ret < 0)
>> +        dev_err(gi3c->se.dev, "%s: error No free addr:%d\n", 
>> __func__, ret);
>> +
>> +    info.dyn_addr = ret;
>> +    info.dcr = I3C_DCR_GENERIC_DEVICE;
>> +    info.bcr = I3C_BCR_I3C_MASTER | I3C_BCR_HDR_CAP;
>> +    info.pid = 0;
>> +
>> +    ret = geni_i3c_clk_map_idx(gi3c);
>> +    if (ret) {
>> +        dev_err(gi3c->se.dev,
>> +            "Invalid clk frequency %d Hz src for %ld Hz bus: %d\n",
>> +            gi3c->clk_src_freq, bus->scl_rate.i3c, ret);
>> +        return ret; //This was missed in upstream : TBD
> 
> why this TBD
> 
Sorry, kept with comment to not miss to add in the final patch upload, 
but missed to remove this comment, i shall remove this for the next 
patch.>> +    }
>> +
>> +    ret = i3c_geni_runtime_get_mutex_lock(gi3c);
>> +    if (ret)
>> +        return ret;
>> +
>> +    qcom_geni_i3c_conf(gi3c, OPEN_DRAIN_MODE);
>> +
>> +    ret = i3c_master_set_info(&gi3c->ctrlr, &info);
>> +    i3c_geni_runtime_put_mutex_unlock(gi3c);
>> +
>> +    return ret;
>> +}
> 
> Thanks,
> Alok