Re: [PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver

Miquel Raynal
2020-08-12T15:18:14Z

Hi Conor, Rajeev,

> +static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> + struct i3c_dev_desc *dev)
> +{
> + struct svc_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> + struct i3c_ibi_slot *slot;
> + unsigned int count;
> + u32 mdatactrl;
> + int ret = 0;
> + u8 *buf;
> +
> + spin_lock(&master->ibi.lock);
> +
> + slot = i3c_generic_ibi_get_free_slot(data->ibi_pool);
> + if (!slot) {
> + ret = -ENOSPC;
> + goto unlock;
> + }
> +
> + slot->len = 0;
> + buf = slot->data;
> + while (readl(master->regs + SVC_I3C_MSTATUS) & SVC_I3C_MINT_RXPEND) {
> + mdatactrl = readl(master->regs + SVC_I3C_MDATACTRL);
> + count = SVC_I3C_MDATACTRL_RXCOUNT(mdatactrl);
> + readsl(master->regs + SVC_I3C_MRDATAB, buf, count);

After discussing with Boris, I have a question about the design: is
there a way to differentiate, from a software point of view, from the
data coming as "IBI" and "regular data"?

Let's say the master initiates a read.
The moment after, an IBI is triggered.
The hanlde_ibi() helper is called to read the IBI payload.
While the IBI interrupted the read operation, it also interrupted the
master dequeuing process of the bytes already in the FIFO. From a
software perspective, we might end up reading a "regular byte" from the
handle_ibi() helper expecting an "IBI byte".

Is there some kind of double queue feature which I missed? Or perhaps a
way to discriminate the origin of the data?

> + slot->len += count;
> + buf += count;
> + }
> +
> + i3c_master_queue_ibi(dev, slot);
> +
> +unlock:
> + spin_unlock(&master->ibi.lock);
> + svc_i3c_master_emit_stop(master);
> + svc_i3c_master_flush_fifo(master);
> +
> + return ret;
> +}
> +
> +static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> + bool mandatory_byte)
> +{
> + unsigned int ibi_ack_nack;
> +
> + ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
> + if (mandatory_byte)
> + ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE;
> + else
> + ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
> +
> + writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
> +}
> +
> +static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
> +{
> + writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK |
> + SVC_I3C_MCTRL_IBIRESP_NACK,
> + master->regs + SVC_I3C_MCTRL);
> +}
> +
> +static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> +{
> + struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
> + u32 active = readl(master->regs + SVC_I3C_MINTMASKED);
> + u32 status = readl(master->regs + SVC_I3C_MSTATUS);
> + unsigned int ibitype = SVC_I3C_MSTATUS_IBITYPE(status);
> + unsigned int ibiaddr = SVC_I3C_MSTATUS_IBIADDR(status);
> + struct i3c_dev_desc *dev;
> + bool rdata;
> +
> + if (active & SVC_I3C_MINT_SLVSTART) {
> + writel(SVC_I3C_MINT_SLVSTART, master->regs + SVC_I3C_MSTATUS);
> + writel(SVC_I3C_MCTRL_REQUEST_AUTO_IBI |
> + SVC_I3C_MCTRL_IBIRESP_MANUAL,
> + master->regs + SVC_I3C_MCTRL);
> + return IRQ_HANDLED;
> + }
> +
> + if (!(active & SVC_I3C_MINT_IBIWON))
> + return IRQ_NONE;
> +
> + writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
> +
> + switch (ibitype) {
> + case SVC_I3C_MSTATUS_IBITYPE_IBI:
> + dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
> + if (WARN_ON(!dev)) {
> + svc_i3c_master_nack_ibi(master);
> + break;
> + }
> +
> + rdata = dev->info.bcr & I3C_BCR_IBI_PAYLOAD;
> + svc_i3c_master_ack_ibi(master, rdata);
> + if (rdata) {
> + svc_i3c_master_disable_interrupts(master);
> + return IRQ_WAKE_THREAD;
> + }
> +
> + break;
> + case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
> + svc_i3c_master_nack_ibi(master);
> + break;
> + case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
> + svc_i3c_master_ack_ibi(master, false);
> + queue_work(master->base.wq, &master->hj_work);
> + break;
> + default:
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t svc_i3c_master_threaded_handler(int irq, void *dev_id)
> +{
> + struct svc_i3c_master *master = (struct svc_i3c_master *)dev_id;
> + u32 status = readl(master->regs + SVC_I3C_MSTATUS);
> + unsigned int ibiaddr = SVC_I3C_MSTATUS_IBIADDR(status);
> + struct i3c_dev_desc *dev;
> +
> + dev = svc_i3c_master_dev_from_addr(master, ibiaddr);
> + if (WARN_ON(!dev)) {
> + svc_i3c_master_emit_stop(master);
> + svc_i3c_master_flush_fifo(master);
> + return IRQ_HANDLED;
> + }
> +
> + svc_i3c_master_handle_ibi(master, dev);
> + svc_i3c_master_enable_interrupts(master,
> + SVC_I3C_MINT_SLVSTART |
> + SVC_I3C_MINT_IBIWON);
> +
> + return IRQ_HANDLED;
> +}

Thanks,
Miquèl