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

[Konrad Dybcio]

On 4/14/25 7:51 AM, Mukesh Kumar Savaliya wrote:
> Thanks Konrad for detailed review.
> 
> 
> On 4/12/2025 4:45 AM, Konrad Dybcio wrote:
>> 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>
>>> ---

[...]

>>> +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
>>
> i see all i3c vendors are using same definitions, so for now can we keep it similar way if it's minor ?

potayto/potahto, let's keep it as is


>>> +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",
> Sure, Reversed with natural and positive check.
> I didn't get about debug log suggestion. Do you want to optimize it to one for both if/else condition ?

Oh no that's me fighting with a bug in thunderbird adding random newlines
to my message.. I only meant the if-condition

>>> +            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?
>>
> DMA mapping can fail OR input validation can also fail. So we want to continue with FIFO mode.


>>> +                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?
>>
> Good find, it's common timeout error. Removed FIFO word.
>> [...]
>>
>>> +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
>>
> Sure, Done.
>>> +
>>> +        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;
>>
> Yes, Done.
>>> +                    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
>>
> Sure, Done.
>>> +            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?
>>
> Yes, thats right.
> It can have either 0xfe OR 0xff.
> 
> Mainly for error detection purpose. This parity bit in tx_buf[0] is set correctly based on nos set bits in the Masked addr is odd or even.
> I have simplified it using parity8().

OK, nice

>>> +
>>> +        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
>>
> Sorry, Didn't get it where to keep bool ?

I blame thunderbird again, I can't find what I meant, it's probably
not super important

Konrad