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

From: Konrad Dybcio
Date: Fri Apr 11 2025 - 19:16:12 EST

Next message: Paul Moore: "Re: [PATCH v2 1/4] security: Hornet LSM"
Previous message: Jacek Kowalski: "Re: [Intel-wired-lan] [PATCH] e1000e: add option not to verify NVM checksum"
In reply to: Mukesh Kumar Savaliya: "[PATCH v4 2/3] i3c: master: Add Qualcomm I3C controller driver"
Next in thread: Mukesh Kumar Savaliya: "Re: [PATCH v4 2/3] i3c: master: Add Qualcomm I3C controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/11/25 1:35 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 <quic_msavaliy@xxxxxxxxxxx>
> ---

[...]

> +#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

lowercase hex, please

> +#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)

' ' after "define", '\t' after

> +#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 SLV_ADDR_SHFT 9

The shift is now unused

> +
> +#define CCC_HDR_CMD_MSK GENMASK(23, 16)
> +#define CCC_HDR_CMD_SHFT 16

And this one *should* be unused, fill in the value with FIELD_PREP instead

> +#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 KHZ(freq) (1000 * (freq))

You can store the values in kHz and do (x * HZ_PER_KHZ) where they're
used instead

> +#define PACKING_BYTES_PW 4

PW -> "PER_WORD", if that's what it means, please

> +#define XFER_TIMEOUT 250
> +#define DFS_INDEX_MAX 7
> +
> +#define I3C_ADDR_MASK 0x7f

This could go to some i3c common header, IIUC it's part of the spec

[...]

> +static inline struct geni_i3c_dev *to_geni_i3c_master(struct i3c_master_controller
> + *master)
> +{
> + return container_of(master, struct geni_i3c_dev, ctrlr);
> +}

#define instead

> +
> +struct geni_i3c_clk_settings {
> + u32 clk_freq_out;
> + u32 clk_src_freq;
> + u8 clk_div;
> + u8 i2c_t_high_cnt;
> + u8 i2c_t_low_cnt;
> + u8 i3c_t_high_cnt;
> + u8 i3c_t_cycle_cnt;
> + u8 i2c_t_cycle_cnt;
> +};
> +
> +/*
> + * Hardware uses the underlying formula to calculate time periods of
> + * SCL clock cycle. Firmware uses some additional cycles excluded from the
> + * below formula and it is confirmed that the time periods are within
> + * specification limits.
> + *
> + * time of high period of I2C SCL:
> + * i2c_t_high = (i2c_t_high_cnt * clk_div) / source_clock
> + * time of low period of I2C SCL:
> + * i2c_t_low = (i2c_t_low_cnt * clk_div) / source_clock
> + * time of full period of I2C SCL:
> + * i2c_t_cycle = (i2c_t_cycle_cnt * clk_div) / source_clock
> + * time of high period of I3C SCL:
> + * i3c_t_high = (i3c_t_high_cnt * clk_div) / source_clock
> + * time of full period of I3C SCL:
> + * i3c_t_cycle = (i3c_t_cycle_cnt * clk_div) / source_clock
> + * clk_freq_out = t / t_cycle
> + */
> +static const struct geni_i3c_clk_settings geni_i3c_clk_map[] = {
> +/* op-freq, src-freq, div, i2c_high, i2c_low, i3c_high, i3c_cyc i2c_cyc */
> + { KHZ(100), KHZ(19200), 1, 76, 90, 7, 8, 192},
> + { KHZ(400), KHZ(19200), 1, 12, 24, 7, 8, 48},
> + { KHZ(1000), KHZ(19200), 1, 4, 9, 7, 0, 19},
> + { KHZ(12500), KHZ(100000), 1, 45, 63, 6, 7, 110},

It's gonna take up considerably more space, but you may want to write
this using C99 designated initializers:

{
.op_freq = ...,
.src_freq = ...,
}, {
...
}

etc.

> +};
> +
> +static int geni_i3c_clk_map_idx(struct geni_i3c_dev *gi3c)
> +{
> + const struct geni_i3c_clk_settings *clk_idx = geni_i3c_clk_map;
> + struct i3c_master_controller *m = &gi3c->ctrlr;
> + struct i3c_bus *bus = i3c_master_get_bus(m);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(geni_i3c_clk_map); i++, clk_idx++) {
> + if ((!bus || clk_idx->clk_freq_out == bus->scl_rate.i3c) &&

Move this nullcheck out of the loop, please.
Or get rid of it, the master registration ensures that there's a bus bound
to it, IIUC.

> + clk_idx->clk_src_freq == gi3c->clk_src_freq) {
> + gi3c->clk_cfg = clk_idx;
> + }
> +
> + if (clk_idx->clk_freq_out == bus->scl_rate.i2c)
> + gi3c->clk_od_cfg = clk_idx;
> + }
> +
> + if (!gi3c->clk_cfg || !gi3c->clk_od_cfg)
> + return -EINVAL;

IIUC this makes the 100 kHz configuration invalid (index 0)

> +
> + return 0;
> +}
> +
> +static void set_new_addr_slot(unsigned long *addrslot, u8 addr)
> +{
> + if (addr > I3C_ADDR_MASK)
> + return;
> +
> + set_bit(addr, addrslot);
> +}

Inline this single-use function, please

> +
> +static void clear_new_addr_slot(unsigned long *addrslot, u8 addr)
> +{
> + if (addr > I3C_ADDR_MASK)
> + return;
> +
> + clear_bit(addr, addrslot);
> +}

And this one

> +
> +static bool is_new_addr_slot_set(unsigned long *addrslot, u8 addr)
> +{
> + if (addr > I3C_ADDR_MASK)
> + return false;

This condition is impossible given the user

> +
> + return test_bit(addr, addrslot);

Inline this check

[...]

> +static void qcom_geni_i3c_conf(struct geni_i3c_dev *gi3c, enum i3c_bus_phase bus_phase)
> +{
> + const struct geni_i3c_clk_settings *clk_idx = gi3c->clk_cfg;
> + unsigned long freq;
> + u32 val, dfs_idx;
> + int ret;
> +
> + if (bus_phase == OPEN_DRAIN_MODE)
> + clk_idx = gi3c->clk_od_cfg;
> +
> + ret = geni_se_clk_freq_match(&gi3c->se, clk_idx->clk_src_freq,
> + &dfs_idx, &freq, false);
> + if (ret)
> + dfs_idx = 0;

Shouldn't we throw some sort of error here instead?

[...]

> +static void geni_i3c_handle_err(struct geni_i3c_dev *gi3c, u32 status)
> +{
> + dev_dbg(gi3c->se.dev, "len:%d, %s\n",
> + gi3c->cur_len, gi3c->cur_is_write ? "write" : "read");
> +
> + if (status & M_GP_IRQ_0_EN) {
> + gi3c->err = -EINVAL;
> + dev_dbg(gi3c->se.dev, "%s\n", "I3C slave early read termination");

Whenever there's just a string literal, please just print it directly.

> + }

Please separate these with a newline

> + if (status & M_GP_IRQ_1_EN) {
> + gi3c->err = -ENOTCONN;
> + dev_dbg(gi3c->se.dev, "%s\n", "NACK: slave unresponsive, check power/reset");
> + }

[...]

> +static irqreturn_t geni_i3c_irq(int irq, void *dev)
> +{
> + u32 m_stat_mask, dm_tx_st, dm_rx_st, m_stat, rx_st, dma;
> + struct geni_i3c_dev *gi3c = dev;
> + unsigned long flags;
> + int j;
> +
> + m_stat = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_STATUS);
> + m_stat_mask = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_EN);
> + rx_st = readl_relaxed(gi3c->se.base + SE_GENI_RX_FIFO_STATUS);
> + dm_tx_st = readl_relaxed(gi3c->se.base + SE_DMA_TX_IRQ_STAT);
> + dm_rx_st = readl_relaxed(gi3c->se.base + SE_DMA_RX_IRQ_STAT);
> + dma = readl_relaxed(gi3c->se.base + SE_GENI_DMA_MODE_EN);
> +
> + spin_lock_irqsave(&gi3c->irq_lock, flags);
> +
> + if (m_stat & SE_I3C_ERR || dm_rx_st & DM_I3C_CB_ERR) {
> + geni_i3c_handle_err(gi3c, m_stat);
> + /* Disable the TX Watermark interrupt to stop TX */
> + if (!dma)
> + writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> + goto irqret;
> + }
> +
> + if (dma) {
> + dev_dbg(gi3c->se.dev, "i3c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, dm_rx_st);
> + goto irqret;
> + }
> +
> + if ((m_stat &
> + (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) &&
> + !gi3c->cur_is_write && gi3c->cur_buf) {

The indentation here is confusing, please align the `(M_RX..` with `m_stat &`

> + u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
> +
> + for (j = 0; j < rxcnt; j++) {
> + u32 val;
> + int p = 0;
> +
> + val = readl_relaxed(gi3c->se.base + SE_GENI_RX_FIFOn);
> + while (gi3c->cur_idx < gi3c->cur_len && p < sizeof(val)) {

sizeof(val) is 4, please unroll this loop into something more readable

> + gi3c->cur_buf[gi3c->cur_idx++] = val & 0xff;
> + val >>= 8;
> + p++;
> + }
> + if (gi3c->cur_idx == gi3c->cur_len)
> + break;
> + }
> + } else if (m_stat & M_TX_FIFO_WATERMARK_EN &&
> + gi3c->cur_is_write && gi3c->cur_buf) {
> + for (j = 0; j < gi3c->tx_wm; j++) {
> + u32 temp;
> + u32 val = 0;
> + int p = 0;
> +
> + while (gi3c->cur_idx < gi3c->cur_len && p < sizeof(val)) {

ditto

> + temp = gi3c->cur_buf[gi3c->cur_idx++];
> + val |= temp << (p * 8);
> + p++;
> + }
> +
> + writel_relaxed(val, gi3c->se.base + SE_GENI_TX_FIFOn);
> + if (gi3c->cur_idx == gi3c->cur_len) {
> + writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> + break;
> + }
> + }
> + }
> +
> +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 ||

odd '\t'

> + dm_rx_st & RX_RESET_DONE) {
> + complete(&gi3c->done);
> + }
> +
> + spin_unlock_irqrestore(&gi3c->irq_lock, flags);
> + return IRQ_HANDLED;

a '\n' before return is customary

[...]

> +static int _i3c_geni_execute_command(struct geni_i3c_dev *gi3c, struct geni_i3c_xfer_params *xfer)
> +{
> + bool is_write = gi3c->cur_is_write;
> + dma_addr_t tx_dma = 0, rx_dma = 0;
> + unsigned long time_remaining;
> + u32 len = gi3c->cur_len;
> + int ret;
> +
> + geni_se_select_mode(&gi3c->se, xfer->mode);
> +
> + gi3c->err = 0;
> + gi3c->cur_idx = 0;
> +
> + if (!is_write) {

Nit: if (is_write) {} .. else {} is more natural> + dev_dbg(gi3c->se.dev, "I3C cmd:0x%x param:0x%x READ len:%d\n",
> + xfer->m_cmd, xfer->m_param, len);
> + writel_relaxed(len, gi3c->se.base + SE_I3C_RX_TRANS_LEN);
> + geni_se_setup_m_cmd(&gi3c->se, xfer->m_cmd, xfer->m_param);
> + if (xfer->mode == GENI_SE_DMA) {
> + ret = geni_se_rx_dma_prep(&gi3c->se, gi3c->cur_buf, len, &rx_dma);
> + if (ret) {
Why would it fail? And why should we fall back silently to FIFO mode then?

> + xfer->mode = GENI_SE_FIFO;
> + geni_se_select_mode(&gi3c->se, xfer->mode);
> + }
> + }
> + } else {
> + dev_dbg(gi3c->se.dev, "I3C cmd:0x%x param:0x%x WRITE len:%d\n",
> + xfer->m_cmd, xfer->m_param, len);
> +
> + writel_relaxed(len, gi3c->se.base + SE_I3C_TX_TRANS_LEN);
> + geni_se_setup_m_cmd(&gi3c->se, xfer->m_cmd, xfer->m_param);
> +
> + if (xfer->mode == GENI_SE_DMA) {
> + ret = geni_se_tx_dma_prep(&gi3c->se, gi3c->cur_buf, len, &tx_dma);
> + if (ret) {
> + xfer->mode = GENI_SE_FIFO;
> + geni_se_select_mode(&gi3c->se, xfer->mode);
> + }
> + }
> +
> + if (xfer->mode == GENI_SE_FIFO && len > 0) /* Get FIFO IRQ */
> + writel_relaxed(1, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> + }
> +
> + time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
> + if (!time_remaining) {
> + unsigned long flags;
> +
> + dev_dbg(gi3c->se.dev, "Timeout completing FIFO transfer\n");

Can it not be DMA mode here too?

[...]

> +static void geni_i3c_perform_daa(struct geni_i3c_dev *gi3c)
> +{
> + u8 last_dyn_addr = 0;
> + int ret;
> +
> + while (1) {
> + u8 rx_buf[8], tx_buf[8];
> + struct geni_i3c_xfer_params xfer = { GENI_SE_FIFO };
> + struct i3c_device_info info = { 0 };
> + struct i3c_dev_desc *i3cdev;
> + bool new_device = true;
> + u64 pid;
> + u8 bcr, dcr, addr;
> +
> + xfer.m_cmd = I2C_READ;
> + xfer.m_param = STOP_STRETCH | CONTINUOUS_MODE_DAA | USE_7E;
> + ret = i3c_geni_execute_read_command(gi3c, &xfer, rx_buf, 8);
> + if (ret)
> + break;
> +
> + dcr = rx_buf[7];
> + bcr = rx_buf[6];
> + pid = ((u64)rx_buf[0] << 40) |
> + ((u64)rx_buf[1] << 32) |
> + ((u64)rx_buf[2] << 24) |
> + ((u64)rx_buf[3] << 16) |
> + ((u64)rx_buf[4] << 8) |
> + ((u64)rx_buf[5]);

FIELD_PREP + GENMASK, please

> +
> + i3c_bus_for_each_i3cdev(&gi3c->ctrlr.bus, i3cdev) {
> + i3c_device_get_info(i3cdev->dev, &info);
> + if (pid == info.pid && dcr == info.dcr && bcr == info.bcr) {
> + new_device = false;
> + addr = (info.dyn_addr) ? info.dyn_addr :

addr = info.dyn_addr ?: info.static_addr;

> + info.static_addr;
> + break;
> + }
> + }
> +
> + if (new_device) {
> + ret = i3c_master_get_free_addr(&gi3c->ctrlr, last_dyn_addr + 1);
> + if (ret < 0)
> + break;
> + addr = (u8)ret;
> + last_dyn_addr = (u8)ret;

nit: while logically the same, last_dyn_addr = addr would make sense here

> + set_new_addr_slot(gi3c->newaddrslots, addr);
> + }
> +

suppose addr=0x38

> + tx_buf[0] = (addr & I3C_ADDR_MASK) << 1;

tx_buf[0] = (0x38 & 0x7f) << 1 = 0x38<<1 = 0x70 = 0b1110000

> + tx_buf[0] |= ~(hweight8(addr & I3C_ADDR_MASK) & 1);

0x70 | ~(hweight8(0x70 & 0x7f) & 1) = 0x70 | ~(3 & 1) = 0x70 | ~BIT(0) = 0xfe

is that the intended result?

> +
> + xfer.m_cmd = I2C_WRITE;
> + xfer.m_param = STOP_STRETCH | BYPASS_ADDR_PHASE | USE_7E;
> +
> + ret = i3c_geni_execute_write_command(gi3c, &xfer, tx_buf, 1);
> + if (ret)
> + break;
> + }
> +}
> +
> +static int geni_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
> + struct i3c_ccc_cmd *cmd)
> +{
> + struct geni_i3c_dev *gi3c = to_geni_i3c_master(m);
> + int i, ret;
> +
> + if (!(cmd->id & I3C_CCC_DIRECT) && cmd->ndests != 1)
> + return -EINVAL;
> +
> + ret = i3c_geni_runtime_get_mutex_lock(gi3c);
> + if (ret)
> + return ret;
> +
> + qcom_geni_i3c_conf(gi3c, OPEN_DRAIN_MODE);
> + for (i = 0; i < cmd->ndests; i++) {
> + int stall = (i < (cmd->ndests - 1)) ||
> + (cmd->id == I3C_CCC_ENTDAA);

bool

Konrad

Next message: Paul Moore: "Re: [PATCH v2 1/4] security: Hornet LSM"
Previous message: Jacek Kowalski: "Re: [Intel-wired-lan] [PATCH] e1000e: add option not to verify NVM checksum"
In reply to: Mukesh Kumar Savaliya: "[PATCH v4 2/3] i3c: master: Add Qualcomm I3C controller driver"
Next in thread: Mukesh Kumar Savaliya: "Re: [PATCH v4 2/3] i3c: master: Add Qualcomm I3C controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]