[PATCH] RapidIO: Add mport driver for Tsi721 bridge

Bounine, Alexandre Alexandre.Bounine at idt.com
Thu Aug 18 06:21:43 EST 2011


Andrew Morton wrote:

> On Fri, 12 Aug 2011 15:45:34 -0400
> Alexandre Bounine <alexandre.bounine at idt.com> wrote:
> 
> > Add RapidIO mport driver for IDT TSI721 PCI Express-to-SRIO bridge
device.
> 
> What a huge driver.

It is not over yet. We have plans to add more stuff later.
 
> >
> > ...
> > +config RAPIDIO_TSI721
> > +	bool "IDT Tsi721 PCI Express SRIO Controller support"
> > +	depends on RAPIDIO && PCI && PCIEPORTBUS
> 
> The dependency on PCI is redundant - PCIEPORTBUS already depends on
> PCI.  Doesn't matter much though.

Agree. Will clean it for next release. 
 
> > +	default "n"
> > +	---help---
> > +	  Include support for IDT Tsi721 PCI Express Serial RapidIO
> controller.
> >
> > ...
> >
> > +	/* Wait until DMA transfer is finished */
> > +	while ((ch_stat = ioread32(priv->regs +
> > +		TSI721_DMAC_STS(TSI721_DMACH_MAINT))) &
TSI721_DMAC_STS_RUN) {
> > +		udelay(10);
> > +		i++;
> > +		if (i >= 5000000) {
> > +			dev_dbg(&priv->pdev->dev,
> > +				"%s : DMA[%d] read timeout
ch_status=%x\n",
> > +				__func__, TSI721_DMACH_MAINT, ch_stat);
> > +			if (!do_wr)
> > +				*data = 0xffffffff;
> > +			err = -EFAULT;
> > +			goto err_out;
> > +		}
> > +	}
> 
> Fifty seconds!?!?

Should be "udelay(1)" - 10 usec is too much here.
 
> EFAULT seems an inappropriate errno.

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

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

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

> > ...
> >
> > +static int
> > +tsi721_pw_handler(struct rio_mport *mport)
> > +{
> > +	struct tsi721_device *priv = mport->priv;
> > +	u32 pw_stat;
> > +	u32 pw_buf[TSI721_RIO_PW_MSG_SIZE/sizeof(u32)];
> > +
> > +
> > +	pw_stat = ioread32(priv->regs + TSI721_RIO_PW_RX_STAT);
> > +
> > +	if (pw_stat & TSI721_RIO_PW_RX_STAT_PW_VAL) {
> > +		pw_buf[0] = ioread32(priv->regs +
TSI721_RIO_PW_RX_CAPT(0));
> > +		pw_buf[1] = ioread32(priv->regs +
TSI721_RIO_PW_RX_CAPT(1));
> > +		pw_buf[2] = ioread32(priv->regs +
TSI721_RIO_PW_RX_CAPT(2));
> > +		pw_buf[3] = ioread32(priv->regs +
TSI721_RIO_PW_RX_CAPT(3));
> > +
> > +		/* Queue PW message (if there is room in FIFO),
> > +		 * otherwise discard it.
> > +		 */
> > +		spin_lock(&priv->pw_fifo_lock);
> > +		if (kfifo_avail(&priv->pw_fifo) >=
TSI721_RIO_PW_MSG_SIZE)
> > +			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,
> > +		  priv->regs + TSI721_RIO_PW_RX_STAT);
> > +
> > +	schedule_work(&priv->pw_work);
> 
> 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.
 
> > +	return 0;
> > +}
> > +
> > +static void tsi721_pw_dpc(struct work_struct *work)
> > +{
> > +	struct tsi721_device *priv = container_of(work, struct
tsi721_device,
> > +						    pw_work);
> > +	unsigned long flags;
> > +	u32 msg_buffer[RIO_PW_MSG_SIZE/sizeof(u32)]; /* Use full size PW
message
> > +							buffer for RIO
layer */
> > +
> > +	/*
> > +	 * 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 = 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 += 4;
> > +		}
> > +		pr_debug("\n");
> > +		}
> > +#endif
> > +		/* Pass the port-write message to RIO core for
processing */
> > +		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);
> > +}
> 
> 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 
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.     
 
> >
> > ...
> >
> > +static void tsi721_db_dpc(struct work_struct *work)
> > +{
> > +	struct tsi721_device *priv = container_of(work, struct
tsi721_device,
> > +						    idb_work);
> > +	struct rio_mport *mport;
> > +	struct rio_dbell *dbell;
> > +	int found = 0;
> > +	u32 wr_ptr, rd_ptr;
> > +	u64 *idb_entry;
> > +	u32 regval;
> > +	union {
> > +		u64 msg;
> > +		u8  bytes[8];
> > +	} idb;
> > +
> > +	/*
> > +	 * Process queued inbound doorbells
> > +	 */
> > +	mport = priv->mport;
> > +
> > +	wr_ptr = ioread32(priv->regs + TSI721_IDQ_WP(IDB_QUEUE));
> > +	rd_ptr = ioread32(priv->regs + TSI721_IDQ_RP(IDB_QUEUE));
> > +
> > +	while (wr_ptr != rd_ptr) {
> > +		idb_entry = (u64 *)(priv->idb_base +
> > +					(TSI721_IDB_ENTRY_SIZE *
rd_ptr));
> > +		rd_ptr++;
> > +		idb.msg = *idb_entry;
> 
> Is this code correct on both little-endian and big-endian hardware?

Yes. It works on both. Tested on x86 and powerpc.
 
> > +		*idb_entry = 0;
> > +
> > +		/* Process one doorbell */
> > +		list_for_each_entry(dbell, &mport->dbells, node) {
> > +			if ((dbell->res->start <= DBELL_INF(idb.bytes))
&&
> > +			    (dbell->res->end >= 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 = TSI721_DEV_INT_SRIO;
> > +	else
> > +		intr = 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);
> 
> 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.

> > +}
> > +
> > +/**
> > + * 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
> > + * immediately after mport initialization. Messaging interrupt
service routines
> > + * should be registered during corresponding open requests.
> > + */
> > +static int tsi721_request_msix(struct rio_mport *mport)
> > +{
> > +	struct tsi721_device *priv = mport->priv;
> > +	int err = 0;
> > +
> > +	err = request_irq(priv->msix[TSI721_VECT_IDB].vector,
> > +			tsi721_sr2pc_ch_msix, 0,
> > +			priv->msix[TSI721_VECT_IDB].irq_name, (void
*)mport);
> > +	if (err)
> > +		goto out;
> > +
> > +	err = request_irq(priv->msix[TSI721_VECT_PWRX].vector,
> > +			tsi721_srio_msix, 0,
> > +			priv->msix[TSI721_VECT_PWRX].irq_name, (void
> *)mport);
> 
> Did we leak the first IRQ here?

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

> 
> > +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 =
TSI721_MSIX_SR2PC_IDBQ_RCV(IDB_QUEUE);
> > +	entries[TSI721_VECT_PWRX].entry = TSI721_MSIX_SRIO_MAC_INT;
> > +
> > +	/*
> > +	 * Initialize MSI-X entries for Messaging Engine:
> > +	 * this driver supports four RIO mailboxes (inbound and
outbound)
> > +	 * NOTE: Inbound message MBOX 0...4 use IB channels 4...7.
Therefore
> > +	 * offset +4 is added to IB MBOX number.
> > +	 */
> > +	for (i = 0; i < RIO_MAX_MBOX; i++) {
> > +		entries[TSI721_VECT_IMB0_RCV + i].entry =
> > +					TSI721_MSIX_IMSG_DQ_RCV(i + 4);
> > +		entries[TSI721_VECT_IMB0_INT + i].entry =
> > +					TSI721_MSIX_IMSG_INT(i + 4);
> > +		entries[TSI721_VECT_OMB0_DONE + i].entry =
> > +					TSI721_MSIX_OMSG_DONE(i);
> > +		entries[TSI721_VECT_OMB0_INT + i].entry =
> > +					TSI721_MSIX_OMSG_INT(i);
> > +	}
> > +
> > +	err = 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 =
entries[TSI721_VECT_IDB].vector;
> > +	snprintf(priv->msix[TSI721_VECT_IDB].irq_name,
IRQ_DEVICE_NAME_MAX,
> > +		 DRV_NAME "-idb at pci:%s", pci_name(priv->pdev));
> > +	priv->msix[TSI721_VECT_PWRX].vector =
entries[TSI721_VECT_PWRX].vector;
> > +	snprintf(priv->msix[TSI721_VECT_PWRX].irq_name,
IRQ_DEVICE_NAME_MAX,
> > +		 DRV_NAME "-pwrx at pci:%s", pci_name(priv->pdev));
> > +
> > +	for (i = 0; i < RIO_MAX_MBOX; i++) {
> > +		priv->msix[TSI721_VECT_IMB0_RCV + i].vector =
> > +				entries[TSI721_VECT_IMB0_RCV +
i].vector;
> > +		snprintf(priv->msix[TSI721_VECT_IMB0_RCV + i].irq_name,
> > +			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-imbr%d at pci:%s",
> > +			 i, pci_name(priv->pdev));
> > +
> > +		priv->msix[TSI721_VECT_IMB0_INT + i].vector =
> > +				entries[TSI721_VECT_IMB0_INT +
i].vector;
> > +		snprintf(priv->msix[TSI721_VECT_IMB0_INT + i].irq_name,
> > +			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-imbi%d at pci:%s",
> > +			 i, pci_name(priv->pdev));
> > +
> > +		priv->msix[TSI721_VECT_OMB0_DONE + i].vector =
> > +				entries[TSI721_VECT_OMB0_DONE +
i].vector;
> > +		snprintf(priv->msix[TSI721_VECT_OMB0_DONE + i].irq_name,
> > +			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-ombd%d at pci:%s",
> > +			 i, pci_name(priv->pdev));
> > +
> > +		priv->msix[TSI721_VECT_OMB0_INT + i].vector =
> > +				entries[TSI721_VECT_OMB0_INT +
i].vector;
> > +		snprintf(priv->msix[TSI721_VECT_OMB0_INT + i].irq_name,
> > +			 IRQ_DEVICE_NAME_MAX, DRV_NAME "-ombi%d at pci:%s",
> > +			 i, pci_name(priv->pdev));
> > +	}
> 
> 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.

> 
> > +	return 0;
> > +}
> > +
> >
> > ...
> >
> > +static int tsi721_bdma_ch_init(struct tsi721_device *priv, int
chnum)
> > +{
> > +	struct tsi721_dma_desc *bd_ptr;
> > +	u64		*sts_ptr;
> > +	dma_addr_t	bd_phys, sts_phys;
> > +	int		sts_size;
> > +	int		bd_num = priv->bdma[chnum].bd_num;
> > +
> > +	dev_dbg(&priv->pdev->dev, "Init Block DMA Engine, CH%d\n",
chnum);
> > +
> > +	/*
> > +	 * Initialize DMA channel for maintenance requests
> > +	 */
> > +
> > +	/* Allocate space for DMA descriptors */
> > +	bd_ptr = dma_alloc_coherent(&priv->pdev->dev,
> > +					bd_num * sizeof(struct
tsi721_dma_desc),
> > +					&bd_phys, GFP_KERNEL);
> > +	if (!bd_ptr)
> > +		return -ENOMEM;
> > +
> > +	priv->bdma[chnum].bd_phys = bd_phys;
> > +	priv->bdma[chnum].bd_base = bd_ptr;
> > +
> > +	memset(bd_ptr, 0, bd_num * sizeof(struct tsi721_dma_desc));
> 
> There it is again.  Perhaps we need a dma_zalloc_coherent().

It will be nice match to kzalloc().
 
> > +	dev_dbg(&priv->pdev->dev, "DMA descriptors @ %p (phys =
%llx)\n",
> > +		bd_ptr, (unsigned long long)bd_phys);
> > +
> > +	/* Allocate space for descriptor status FIFO */
> > +	sts_size = (bd_num >= TSI721_DMA_MINSTSSZ) ?
> > +					bd_num : TSI721_DMA_MINSTSSZ;
> > +	sts_size = roundup_pow_of_two(sts_size);
> > +	sts_ptr = dma_alloc_coherent(&priv->pdev->dev,
> > +				     sts_size * sizeof(struct
tsi721_dma_sts),
> > +				     &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),
> > +				  bd_ptr, bd_phys);
> > +		priv->bdma[chnum].bd_base = NULL;
> > +		return -ENOMEM;
> > +	}
> > +
> > +	priv->bdma[chnum].sts_phys = sts_phys;
> > +	priv->bdma[chnum].sts_base = sts_ptr;
> > +	priv->bdma[chnum].sts_size = sts_size;
> > +
> > +	memset(sts_ptr, 0, sts_size);
> 
> and again.

This memset() may be safely removed. I will review all similar cases.
 
> >
> > ...
> >
> > +struct tsi721_dma_desc {
> > +	__le32 type_id;
> > +

...
> > +
> > +	union {
> > +		struct {		   /* if DTYPE == 1 */
> > +			__le32 bufptr_lo;
> > +			__le32 bufptr_hi;
> > +			__le32 s_dist;
> > +			__le32 s_size;
> > +		} t1;
> > +		__le32 data[4];		   /* if DTYPE == 2 */
> > +		u32    reserved[4];	   /* if DTYPE == 3 */
> > +	};
> > +} __attribute__((aligned(32)));
> 
> We have the __aligned helper for this.  (more below)

No excuse to ignore it. Will update.
 
> >
> > ...
> >
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


More information about the Linuxppc-dev mailing list