RE: [PATCH] RapidIO: Add mport driver for Tsi721 bridge

Andrew Morton wrote:

On Fri, 12 Aug 2011 15:45:34 -0400
Alexandre Bounine [off-list ref] wrote:
=20

quoted

Add RapidIO mport driver for IDT TSI721 PCI Express-to-SRIO bridge

device.

=20
What a huge driver.

It is not over yet. We have plans to add more stuff later.
=20

quoted

...
+config RAPIDIO_TSI721
+	bool "IDT Tsi721 PCI Express SRIO Controller support"
+	depends on RAPIDIO && PCI && PCIEPORTBUS

=20
The dependency on PCI is redundant - PCIEPORTBUS already depends on
PCI.  Doesn't matter much though.

Agree. Will clean it for next release.=20
=20

quoted

+	default "n"
+	---help---
+	  Include support for IDT Tsi721 PCI Express Serial RapidIO

controller.

quoted

...

+	/* Wait until DMA transfer is finished */
+	while ((ch_stat =3D ioread32(priv->regs +
+		TSI721_DMAC_STS(TSI721_DMACH_MAINT))) &

TSI721_DMAC_STS_RUN) {

quoted

+		udelay(10);
+		i++;
+		if (i >=3D 5000000) {
+			dev_dbg(&priv->pdev->dev,
+				"%s : DMA[%d] read timeout

ch_status=3D%x\n",

quoted

+				__func__, TSI721_DMACH_MAINT, ch_stat);
+			if (!do_wr)
+				*data =3D 0xffffffff;
+			err =3D -EFAULT;
+			goto err_out;
+		}
+	}

=20
Fifty seconds!?!?

Should be "udelay(1)" - 10 usec is too much here.
=20

EFAULT seems an inappropriate errno.

Switching to EIO.
=20

quoted

+	if (ch_stat & TSI721_DMAC_STS_ABORT) {
+		/* If DMA operation aborted due to error,
+		 * reinitialize DMA channel
+		 */

.....

quoted

+		iowrite32(0, priv->regs +
+				TSI721_DMAC_DWRCNT(TSI721_DMACH_MAINT));
+		udelay(1);
+		if (!do_wr)
+			*data =3D 0xffffffff;
+		err =3D -EFAULT;

Here EIO as well because ABORT error reports more events than just bad
address.

quoted

...

+static int
+tsi721_pw_handler(struct rio_mport *mport)
+{
+	struct tsi721_device *priv =3D mport->priv;
+	u32 pw_stat;
+	u32 pw_buf[TSI721_RIO_PW_MSG_SIZE/sizeof(u32)];
+
+
+	pw_stat =3D ioread32(priv->regs + TSI721_RIO_PW_RX_STAT);
+
+	if (pw_stat & TSI721_RIO_PW_RX_STAT_PW_VAL) {
+		pw_buf[0] =3D ioread32(priv->regs +

TSI721_RIO_PW_RX_CAPT(0));

quoted

+		pw_buf[1] =3D ioread32(priv->regs +

TSI721_RIO_PW_RX_CAPT(1));

quoted

+		pw_buf[2] =3D ioread32(priv->regs +

TSI721_RIO_PW_RX_CAPT(2));

quoted

+		pw_buf[3] =3D ioread32(priv->regs +

TSI721_RIO_PW_RX_CAPT(3));

quoted

+
+		/* Queue PW message (if there is room in FIFO),
+		 * otherwise discard it.
+		 */
+		spin_lock(&priv->pw_fifo_lock);
+		if (kfifo_avail(&priv->pw_fifo) >=3D

TSI721_RIO_PW_MSG_SIZE)

quoted

+			kfifo_in(&priv->pw_fifo, pw_buf,
+						TSI721_RIO_PW_MSG_SIZE);
+		else
+			priv->pw_discard_count++;
+		spin_unlock(&priv->pw_fifo_lock);
+	}
+
+	/* Clear pending PW interrupts */
+	iowrite32(TSI721_RIO_PW_RX_STAT_PW_DISC |

TSI721_RIO_PW_RX_STAT_PW_VAL,

quoted

+		  priv->regs + TSI721_RIO_PW_RX_STAT);
+
+	schedule_work(&priv->pw_work);

=20
I see scheduled work being done, but no flush_scheduled_work() or
similar.  This is often a bug, leading to code being executed after
device shutdown or even after rmmod.

Possibly my oversight. I will check if/where flush is needed.
=20

quoted

+	return 0;
+}
+
+static void tsi721_pw_dpc(struct work_struct *work)
+{
+	struct tsi721_device *priv =3D container_of(work, struct

tsi721_device,

quoted

+						    pw_work);
+	unsigned long flags;
+	u32 msg_buffer[RIO_PW_MSG_SIZE/sizeof(u32)]; /* Use full size PW

message

quoted

+							buffer for RIO

layer */

quoted

+
+	/*
+	 * Process port-write messages
+	 */
+	spin_lock_irqsave(&priv->pw_fifo_lock, flags);
+	while (kfifo_out(&priv->pw_fifo, (unsigned char *)msg_buffer,
+			 TSI721_RIO_PW_MSG_SIZE)) {
+		/* Process one message */
+		spin_unlock_irqrestore(&priv->pw_fifo_lock, flags);
+#ifdef DEBUG_PW
+		{
+		u32 i;
+		pr_debug("%s : Port-Write Message:", __func__);
+		for (i =3D 0; i < RIO_PW_MSG_SIZE/sizeof(u32); ) {
+			pr_debug("0x%02x: %08x %08x %08x %08x", i*4,
+				msg_buffer[i], msg_buffer[i + 1],
+				msg_buffer[i + 2], msg_buffer[i + 3]);
+			i +=3D 4;
+		}
+		pr_debug("\n");
+		}
+#endif
+		/* Pass the port-write message to RIO core for

processing */

quoted

+		rio_inb_pwrite_handler((union rio_pw_msg *)msg_buffer);
+		spin_lock_irqsave(&priv->pw_fifo_lock, flags);
+	}
+	spin_unlock_irqrestore(&priv->pw_fifo_lock, flags);
+}

=20
The lock handling in this function is pretty ugly-looking.  Is it
correct?

Ugly for sure. I am trying to protect message FIFO and not to lock it
during=20
execution of rio_inb_pwrite_handler() because it may take relatively
long time.
Inbound port-write controller does not have any HW queue to store PW
messages
and my goal is to get them from device ASAP and to place into the FIFO.
I will redo this function to make locking to look better.    =20
=20

quoted

...

+static void tsi721_db_dpc(struct work_struct *work)
+{
+	struct tsi721_device *priv =3D container_of(work, struct

tsi721_device,

quoted

+						    idb_work);
+	struct rio_mport *mport;
+	struct rio_dbell *dbell;
+	int found =3D 0;
+	u32 wr_ptr, rd_ptr;
+	u64 *idb_entry;
+	u32 regval;
+	union {
+		u64 msg;
+		u8  bytes[8];
+	} idb;
+
+	/*
+	 * Process queued inbound doorbells
+	 */
+	mport =3D priv->mport;
+
+	wr_ptr =3D ioread32(priv->regs + TSI721_IDQ_WP(IDB_QUEUE));
+	rd_ptr =3D ioread32(priv->regs + TSI721_IDQ_RP(IDB_QUEUE));
+
+	while (wr_ptr !=3D rd_ptr) {
+		idb_entry =3D (u64 *)(priv->idb_base +
+					(TSI721_IDB_ENTRY_SIZE *

rd_ptr));

quoted

+		rd_ptr++;
+		idb.msg =3D *idb_entry;

=20
Is this code correct on both little-endian and big-endian hardware?

Yes. It works on both. Tested on x86 and powerpc.
=20

quoted

+		*idb_entry =3D 0;
+
+		/* Process one doorbell */
+		list_for_each_entry(dbell, &mport->dbells, node) {
+			if ((dbell->res->start <=3D DBELL_INF(idb.bytes))

&&

quoted

+			    (dbell->res->end >=3D DBELL_INF(idb.bytes))) {
...

+static void tsi721_interrupts_init(struct tsi721_device *priv)
+{
+	u32 intr;
+
+	/* Enable IDB interrupts */
+	iowrite32(TSI721_SR_CHINT_ALL,
+		priv->regs + TSI721_SR_CHINT(IDB_QUEUE));
+	iowrite32(TSI721_SR_CHINT_IDBQRCV,
+		priv->regs + TSI721_SR_CHINTE(IDB_QUEUE));
+	iowrite32(TSI721_INT_SR2PC_CHAN(IDB_QUEUE),
+		priv->regs + TSI721_DEV_CHAN_INTE);
+
+	/* Enable SRIO MAC interrupts */
+	iowrite32(TSI721_RIO_EM_DEV_INT_EN_INT,
+		priv->regs + TSI721_RIO_EM_DEV_INT_EN);
+
+	if (priv->flags & TSI721_USING_MSIX)
+		intr =3D TSI721_DEV_INT_SRIO;
+	else
+		intr =3D TSI721_DEV_INT_SR2PC_CH | TSI721_DEV_INT_SRIO |
+			TSI721_DEV_INT_SMSG_CH;
+
+	iowrite32(intr, priv->regs + TSI721_DEV_INTE);
+	(void)ioread32(priv->regs + TSI721_DEV_INTE);

=20
Why include all of these void casts, btw?

Old habit. Used to have compiler warnings in some situations.
Not applicable to this case though. Will clean-up.

quoted

+}
+
+/**
+ * tsi721_request_msix - register interrupt service for MSI-X mode.
+ * @mport: RapidIO master port structure
+ *
+ * Registers MSI-X interrupt service routines for interrupts that

are active

quoted

+ * immediately after mport initialization. Messaging interrupt

service routines

quoted

+ * should be registered during corresponding open requests.
+ */
+static int tsi721_request_msix(struct rio_mport *mport)
+{
+	struct tsi721_device *priv =3D mport->priv;
+	int err =3D 0;
+
+	err =3D request_irq(priv->msix[TSI721_VECT_IDB].vector,
+			tsi721_sr2pc_ch_msix, 0,
+			priv->msix[TSI721_VECT_IDB].irq_name, (void

*)mport);

quoted

+	if (err)
+		goto out;
+
+	err =3D request_irq(priv->msix[TSI721_VECT_PWRX].vector,
+			tsi721_srio_msix, 0,
+			priv->msix[TSI721_VECT_PWRX].irq_name, (void

*)mport);
=20
Did we leak the first IRQ here?

Yes we did. Will fix it here and in one more place.

=20

quoted

+out:
+	return err;
+}
+

...

+static int tsi721_enable_msix(struct tsi721_device *priv)
+{
+	struct msix_entry entries[TSI721_VECT_MAX];
+	int err;
+	int i;
+
+	entries[TSI721_VECT_IDB].entry =3D

TSI721_MSIX_SR2PC_IDBQ_RCV(IDB_QUEUE);

quoted

+	entries[TSI721_VECT_PWRX].entry =3D TSI721_MSIX_SRIO_MAC_INT;
+
+	/*
+	 * Initialize MSI-X entries for Messaging Engine:
+	 * this driver supports four RIO mailboxes (inbound and

outbound)

quoted

+	 * NOTE: Inbound message MBOX 0...4 use IB channels 4...7.

Therefore

quoted

+	 * offset +4 is added to IB MBOX number.
+	 */
+	for (i =3D 0; i < RIO_MAX_MBOX; i++) {
+		entries[TSI721_VECT_IMB0_RCV + i].entry =3D
+					TSI721_MSIX_IMSG_DQ_RCV(i + 4);
+		entries[TSI721_VECT_IMB0_INT + i].entry =3D
+					TSI721_MSIX_IMSG_INT(i + 4);
+		entries[TSI721_VECT_OMB0_DONE + i].entry =3D
+					TSI721_MSIX_OMSG_DONE(i);
+		entries[TSI721_VECT_OMB0_INT + i].entry =3D
+					TSI721_MSIX_OMSG_INT(i);
+	}
+
+	err =3D pci_enable_msix(priv->pdev, entries, ARRAY_SIZE(entries));
+	if (err) {
+		if (err > 0)
+			dev_info(&priv->pdev->dev,
+				 "Only %d MSI-X vectors available, "
+				 "not using MSI-X\n", err);
+		return err;
+	}
+
+	/*
+	 * Copy MSI-X vector information into tsi721 private structure
+	 */
+	priv->msix[TSI721_VECT_IDB].vector =3D

entries[TSI721_VECT_IDB].vector;

quoted

+	snprintf(priv->msix[TSI721_VECT_IDB].irq_name,

IRQ_DEVICE_NAME_MAX,

quoted

+		 DRV_NAME "-idb@pci:%s", pci_name(priv->pdev));
+	priv->msix[TSI721_VECT_PWRX].vector =3D

entries[TSI721_VECT_PWRX].vector;

quoted

+	snprintf(priv->msix[TSI721_VECT_PWRX].irq_name,

IRQ_DEVICE_NAME_MAX,

quoted

+		 DRV_NAME "-pwrx@pci:%s", pci_name(priv->pdev));
+
+	for (i =3D 0; i < RIO_MAX_MBOX; i++) {
+		priv->msix[TSI721_VECT_IMB0_RCV + i].vector =3D
+				entries[TSI721_VECT_IMB0_RCV +

i].vector;

quoted

+		snprintf(priv->msix[TSI721_VECT_IMB0_RCV + i].irq_name,
+			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-imbr%d@pci:%s",
+			 i, pci_name(priv->pdev));
+
+		priv->msix[TSI721_VECT_IMB0_INT + i].vector =3D
+				entries[TSI721_VECT_IMB0_INT +

i].vector;

quoted

+		snprintf(priv->msix[TSI721_VECT_IMB0_INT + i].irq_name,
+			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-imbi%d@pci:%s",
+			 i, pci_name(priv->pdev));
+
+		priv->msix[TSI721_VECT_OMB0_DONE + i].vector =3D
+				entries[TSI721_VECT_OMB0_DONE +

i].vector;

quoted

+		snprintf(priv->msix[TSI721_VECT_OMB0_DONE + i].irq_name,
+			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-ombd%d@pci:%s",
+			 i, pci_name(priv->pdev));
+
+		priv->msix[TSI721_VECT_OMB0_INT + i].vector =3D
+				entries[TSI721_VECT_OMB0_INT +

i].vector;

quoted

+		snprintf(priv->msix[TSI721_VECT_OMB0_INT + i].irq_name,
+			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-ombi%d@pci:%s",
+			 i, pci_name(priv->pdev));
+	}

=20
Did we need a dependency on CONFIG_PCI_MSI?

To reduce a driver footprint yes, but it will not help if
system implements regular MSI only. This may be better handled by using
device specific config option depending on CONFIG_PCI_MSI. This way I
will be able
to remove MSI-X code while keeping MSI function. Marking as TODO.

If code size is not a concern, pci_enable_msix() has dependency on
CONFIG_PCI_MSI
and will fail if MSI support is not enabled.

=20

quoted

+	return 0;
+}
+

...

+static int tsi721_bdma_ch_init(struct tsi721_device *priv, int

chnum)

quoted

+{
+	struct tsi721_dma_desc *bd_ptr;
+	u64		*sts_ptr;
+	dma_addr_t	bd_phys, sts_phys;
+	int		sts_size;
+	int		bd_num =3D priv->bdma[chnum].bd_num;
+
+	dev_dbg(&priv->pdev->dev, "Init Block DMA Engine, CH%d\n",

chnum);

quoted

+
+	/*
+	 * Initialize DMA channel for maintenance requests
+	 */
+
+	/* Allocate space for DMA descriptors */
+	bd_ptr =3D dma_alloc_coherent(&priv->pdev->dev,
+					bd_num * sizeof(struct

tsi721_dma_desc),

quoted

+					&bd_phys, GFP_KERNEL);
+	if (!bd_ptr)
+		return -ENOMEM;
+
+	priv->bdma[chnum].bd_phys =3D bd_phys;
+	priv->bdma[chnum].bd_base =3D bd_ptr;
+
+	memset(bd_ptr, 0, bd_num * sizeof(struct tsi721_dma_desc));

=20
There it is again.  Perhaps we need a dma_zalloc_coherent().

It will be nice match to kzalloc().
=20

quoted

+	dev_dbg(&priv->pdev->dev, "DMA descriptors @ %p (phys =3D

%llx)\n",

quoted

+		bd_ptr, (unsigned long long)bd_phys);
+
+	/* Allocate space for descriptor status FIFO */
+	sts_size =3D (bd_num >=3D TSI721_DMA_MINSTSSZ) ?
+					bd_num : TSI721_DMA_MINSTSSZ;
+	sts_size =3D roundup_pow_of_two(sts_size);
+	sts_ptr =3D dma_alloc_coherent(&priv->pdev->dev,
+				     sts_size * sizeof(struct

tsi721_dma_sts),

quoted

+				     &sts_phys, GFP_KERNEL);
+	if (!sts_ptr) {
+		/* Free space allocated for DMA descriptors */
+		dma_free_coherent(&priv->pdev->dev,
+				  bd_num * sizeof(struct

tsi721_dma_desc),

quoted

+				  bd_ptr, bd_phys);
+		priv->bdma[chnum].bd_base =3D NULL;
+		return -ENOMEM;
+	}
+
+	priv->bdma[chnum].sts_phys =3D sts_phys;
+	priv->bdma[chnum].sts_base =3D sts_ptr;
+	priv->bdma[chnum].sts_size =3D sts_size;
+
+	memset(sts_ptr, 0, sts_size);

=20
and again.

This memset() may be safely removed. I will review all similar cases.
=20

quoted

...

+struct tsi721_dma_desc {
+	__le32 type_id;
+

...

quoted

+
+	union {
+		struct {		   /* if DTYPE =3D=3D 1 */
+			__le32 bufptr_lo;
+			__le32 bufptr_hi;
+			__le32 s_dist;
+			__le32 s_size;
+		} t1;
+		__le32 data[4];		   /* if DTYPE =3D=3D 2 */
+		u32    reserved[4];	   /* if DTYPE =3D=3D 3 */
+	};
+} __attribute__((aligned(32)));

=20
We have the __aligned helper for this.  (more below)

No excuse to ignore it. Will update.
=20

quoted

...

=20
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev