Re: [PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver
Hi Boris,
It's been quite some time since you made this review, but now that I am
ready to send a new version, I think it is useful to answer your
questions and remarks below which I pretty much all addressed with
significant changes.
Also adding Alexandre so he can smoothly get into this driver :)
> > +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(>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);
> > + slot->len += count;
> > + buf += count;
> > + }
>
> I don't think the while loop and i3c_generic_ibi_get_free_slot() call
> have to be protected by the ibi lock (having an unbounded while loop in
> a critical section makes me nervous).
Dropped indeed, I don't think it is needed as well.
> We also discussed the race you have because IBIs and regular transfers
> share the same RX FIFO, which you mentioned in your previous reply
> already.
I addressed this issue by automatically nacking IBIs happening during
transfers (hardware feature).
> > +
> > + i3c_master_queue_ibi(dev, slot);
> > +
> > +unlock:
> > + spin_unlock(>ibi.lock);
> > + svc_i3c_master_emit_stop(master);
> > + svc_i3c_master_flush_fifo(master);
>
> Flush FIFOs? Does it flush all remaining bytes present in the RX/TX
> FIFOs? Are you sure that's appropriate?
Indeed this is not appropriate and has been dropped.
> > +
> > + 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;
>
>
> IIRC, some devices send more than one byte, does that mean you don't
> support those?
The dequeuing mechanism is able to read up to 16 bytes (FIFO size), it
should not be a problem.
> > + 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);
>
> I would expect some kind of auto-ack/nack mechanism. Not sure what
> happens when you do it manually, but if that blocks the bus waiting for
> the interrupt handler to tell the I3C master what to do with IBI,
> that's far from ideal.
After discussing with people who designed the IP, they told me that
the time between SDA being pulled low to interrupt the master and the
master responding is unbounded. What is critical is the handling of
the interrupt once the master authorized the interrupt. I entirely
reworked the IRQ handling for that in v3.
> > + 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)) {
>
> I wouldn't WARN_ON() that. I wouldn't be surprised if some slaves send
> IBIs without being allowed, and you're in an interrupt context. This
> being said, you should probably trigger a DISEC on this device when
> that happens.
I
Re: [PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver
On Wed, 12 Aug 2020 16:13:11 +0200
Miquel Raynal wrote:
> +
> +#define SVC_I3C_MAX_DEVS 32
> +
> +struct svc_i3c_cmd {
> + u8 addr;
> + bool rnw;
> + u8 *in;
> + const void *out;
> + unsigned int len;
> + unsigned int read_len;
> + bool continued;
> +};
> +
> +struct svc_i3c_xfer {
> + struct list_head node;
> + struct completion comp;
> + int ret;
> + unsigned int type;
> + unsigned int ncmds;
> + struct svc_i3c_cmd cmds[];
> +};
> +
> +/**
> + * struct svc_i3c_master - Silvaco I3C Master structure
> + * @base: I3C master controller
> + * @dev: Corresponding device
> + * @regs: Memory mapping
> + * @free_slots: Bit array of available slots
> + * @addrs: Array containing the dynamic addresses of each attached device
> + * @descs: Array of descriptors, one per attached device
> + * @hj_work: Hot-join work
> + * @irq: Main interrupt
> + * @pclk: System clock
> + * @fclk: Fast clock (bus)
> + * @sclk: Slow clock (other events)
> + * @xferqueue: Transfer queue structure
> + * @xferqueue.list: List member
> + * @xferqueue.cur: Current ongoing transfer
> + * @xferqueue.lock: Queue lock
> + * @ibi: IBI structure
> + * @ibi.num_slots: Number of slots available in @ibi.slots
> + * @ibi.slots: Available IBI slots
> + * @ibi.lock: IBI lock
> + * @bus_lock: Bus lock
> + */
> +struct svc_i3c_master {
> + struct i3c_master_controller base;
> + struct device *dev;
> + void __iomem *regs;
> + u32 free_slots;
> + u8 addrs[SVC_I3C_MAX_DEVS];
> + struct i3c_dev_desc *descs[SVC_I3C_MAX_DEVS];
> + struct work_struct hj_work;
> + int irq;
> + struct clk *pclk;
> + struct clk *fclk;
> + struct clk *sclk;
> + struct {
> + struct list_head list;
> + struct svc_i3c_xfer *cur;
> + /* Prevent races between transfers */
> + spinlock_t lock;
> + } xferqueue;
> + struct {
> + unsigned int num_slots;
> + struct i3c_dev_desc **slots;
> + /* Prevent races within IBI handlers */
> + spinlock_t lock;
> + } ibi;
> +};
> +
> +/**
> + * struct svc_i3c_i3c_dev_data - Device specific data
> + * @index: Index in the master tables corresponding to this device
> + * @ibi: IBI slot index in the master structure
> + * @ibi_pool: IBI pool associated to this device
> + */
> +struct svc_i3c_i2c_dev_data {
> + u8 index;
> + int ibi;
> + struct i3c_generic_ibi_pool *ibi_pool;
> +};
> +
> +static void svc_i3c_master_enable_interrupts(struct svc_i3c_master *master,
> u32 mask)
> +{
> + writel(mask, master->regs + SVC_I3C_MINTSET);
> +}
> +
> +static void svc_i3c_master_disable_interrupts(struct svc_i3c_master *master)
> +{
> + u32 mask = readl(master->regs + SVC_I3C_MINTSET);
> +
> + writel(mask, master->regs + SVC_I3C_MINTCLR);
> +}
> +
> +static inline struct svc_i3c_master *
> +to_svc_i3c_master(struct i3c_master_controller *master)
> +{
> + return container_of(master, struct svc_i3c_master, base);
> +}
> +
> +static void svc_i3c_master_hj(struct work_struct *work)
> +{
> + struct svc_i3c_master *master;
> +
> + master = container_of(work, struct svc_i3c_master, hj_work);
> + i3c_master_do_daa(>base);
> +}
> +
> +static void svc_i3c_master_flush_fifo(struct svc_i3c_master *master)
> +{
> + writel(SVC_I3C_MDATACTRL_FLUSHTB | SVC_I3C_MDATACTRL_FLUSHRB,
> +master->regs + SVC_I3C_MDATACTRL);
> +}
> +
> +static struct i3c_dev_desc *
> +svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
> + unsigned int ibiaddr)
> +{
> + int i;
> +
> + for (i = 0; i < SVC_I3C_MAX_DEVS; i++)
> + if (master->addrs[i] == ibiaddr)
> + break;
> +
> + if (i == SVC_I3C_MAX_DEVS)
> + return NULL;
> +
> + return master->descs[i];
> +}
> +
> +static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
> +{
> + writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
> +}
> +
> +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(>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);
> + slot->len += count;
> + buf += count;
> + }
I don't think the
Re: [PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver
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(>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(>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, >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);
> +
[PATCH v2 3/4] i3c: master: svc: Add Silvaco I3C master driver
Add support for Silvaco I3C dual-role IP. The master role is supported in SDR mode only. I2C transfers have not been tested but are shared because they are so close to the I3C transfers in terms of registers configuration. Signed-off-by: Miquel Raynal --- Changes in v2: * Declared all the functions static (2 were missing). * Catched errors reported by i3c_master_get_free_addr(). * Reworked the IBI handling to avoid sleeping. * Updated the vendor prefix in the compatible and also use "silvaco" instead of "svc" in the driver's name. drivers/i3c/master/Kconfig |8 + drivers/i3c/master/Makefile |1 + drivers/i3c/master/svc-i3c-master.c | 1349 +++ 3 files changed, 1358 insertions(+) create mode 100644 drivers/i3c/master/svc-i3c-master.c diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig index 4e80a1fcbf91..032b4de14277 100644 --- a/drivers/i3c/master/Kconfig +++ b/drivers/i3c/master/Kconfig @@ -21,3 +21,11 @@ config DW_I3C_MASTER This driver can also be built as a module. If so, the module will be called dw-i3c-master. + +config SVC_I3C_MASTER + tristate "Silvaco I3C Dual-Role Master driver" + depends on I3C + depends on HAS_IOMEM + depends on !(ALPHA || PARISC) + help + Support for Silvaco I3C Dual-Role Master Controller. diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile index 7eea9e086144..69e5f5479df9 100644 --- a/drivers/i3c/master/Makefile +++ b/drivers/i3c/master/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o obj-$(CONFIG_DW_I3C_MASTER)+= dw-i3c-master.o +obj-$(CONFIG_SVC_I3C_MASTER) += svc-i3c-master.o diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c new file mode 100644 index ..580400b6501c --- /dev/null +++ b/drivers/i3c/master/svc-i3c-master.c @@ -0,0 +1,1349 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Silvaco dual-role I3C master driver + * + * Copyright (C) 2020 Silvaco + * Author: Miquel RAYNAL + * Based on a work from: Conor Culhane + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* Master Mode Registers */ +#define SVC_I3C_MCONFIG 0x000 +#define SVC_I3C_MCONFIG_MASTER_EN BIT(0) +#define SVC_I3C_MCONFIG_DISTO(x) FIELD_PREP(BIT(3), (x)) +#define SVC_I3C_MCONFIG_HKEEP(x) FIELD_PREP(GENMASK(5, 4), (x)) +#define SVC_I3C_MCONFIG_ODSTOP(x) FIELD_PREP(BIT(6), (x)) +#define SVC_I3C_MCONFIG_PPBAUD(x) FIELD_PREP(GENMASK(11, 8), (x)) +#define SVC_I3C_MCONFIG_PPLOW(x) FIELD_PREP(GENMASK(15, 12), (x)) +#define SVC_I3C_MCONFIG_ODBAUD(x) FIELD_PREP(GENMASK(23, 16), (x)) +#define SVC_I3C_MCONFIG_ODHPP(x) FIELD_PREP(BIT(24), (x)) +#define SVC_I3C_MCONFIG_SKEW(x) FIELD_PREP(GENMASK(27, 25), (x)) +#define SVC_I3C_MCONFIG_I2CBAUD(x) FIELD_PREP(GENMASK(31, 28), (x)) + +#define SVC_I3C_MCTRL0x084 +#define SVC_I3C_MCTRL_REQUEST_START_ADDR1 +#define SVC_I3C_MCTRL_REQUEST_STOP 2 +#define SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK 3 +#define SVC_I3C_MCTRL_REQUEST_PROC_DAA 4 +#define SVC_I3C_MCTRL_REQUEST_AUTO_IBI 7 +#define SVC_I3C_MCTRL_TYPE_I3C 0 +#define SVC_I3C_MCTRL_TYPE_I2C BIT(4) +#define SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE 0 +#define SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE BIT(7) +#define SVC_I3C_MCTRL_IBIRESP_NACK BIT(6) +#define SVC_I3C_MCTRL_IBIRESP_MANUAL GENMASK(7, 6) +#define SVC_I3C_MCTRL_DIR(x) FIELD_PREP(BIT(8), (x)) +#define SVC_I3C_MCTRL_DIR_WRITE 0 +#define SVC_I3C_MCTRL_DIR_READ 1 +#define SVC_I3C_MCTRL_ADDR(x) FIELD_PREP(GENMASK(15, 9), (x)) +#define SVC_I3C_MCTRL_RDTERM(x) FIELD_PREP(GENMASK(23, 16), (x)) + +#define SVC_I3C_MSTATUS 0x088 +#define SVC_I3C_MSTATUS_STATE(x) FIELD_GET(GENMASK(2, 0), (x)) +#define SVC_I3C_MSTATUS_STATE_DAA(x) (SVC_I3C_MSTATUS_STATE(x) == 5) +#define SVC_I3C_MSTATUS_STATE_IDLE(x) (SVC_I3C_MSTATUS_STATE(x) == 0) +#define SVC_I3C_MSTATUS_BETWEEN(x) FIELD_GET(BIT(4), (x)) +#define SVC_I3C_MSTATUS_NACKED(x) FIELD_GET(BIT(5), (x)) +#define SVC_I3C_MSTATUS_IBITYPE(x) FIELD_GET(GENMASK(7, 6), (x)) +#define SVC_I3C_MSTATUS_IBITYPE_IBI 1 +#define SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST 2 +#define SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN 3 +#define SVC_I3C_MSTATUS_MCTRLDONE(x) FIELD_GET(BIT(9), (x)) +#define SVC_I3C_MSTATUS_COMPLETE(x) FIELD_GET(BIT(10), (x)) +#define SVC_I3C_MSTATUS_ERRWARN(x) FIELD_GET(BIT(15), (x)) +#define SVC_I3C_MSTATUS_IBIADDR(x) FIELD_GET(GENMASK(30, 24), (x)) + +#define SVC_I3C_IBIRULES 0x08C +#define SVC_I3C_MINTSET 0x090 +#define SVC_I3C_MINTCLR 0x094 +#define SVC_I3C_MINTMASKED 0x098 +#define SVC_I3C_MINT_SLVSTART BIT(8) +#define SVC_I3C_MINT_MCTRLDONE BIT(9) +#define SVC_I3C_MINT_RXPEND BIT(11) +#define SVC_I3C_MINT_TXNOTFULL BIT(12)
