[PATCH] [POWERPC] 8xx: PQ SoC IRDA support

Samuel Ortiz samuel at sortiz.org
Wed May 9 23:46:11 EST 2007


Hi Vitaly,

On 5/9/2007, "Vitaly Bordug" <vitb at kernel.crashing.org> wrote:
>> I'd really prefer this patch to be splitted in 2 parts: The PPC
>> specific bits, and the IrDA driver that would apply on top of it.
>> Meanwhile, I can comment on the IrDA driver code:
>>
>I used to do such before with network drivers but it may result in badly breakage since stuff is heading through different routes. The most productive way seems to acquire ACK from both driver-wise and arch-wise areas,
>and have Andrew to merge it. IOW, separating stuff that cannot live one without other is error-prone.
>
>Of course, there's np do do that for your convenience - I'm just against pushing it through different trees.
>>
Fine, but please CC netdev and irda-users at lists.sourceforge.net for your
next patch iteration.


>> >+static irqreturn_t mpc8xx_irda_irq(int irq, void *dev_id)
>> >+{
>> >+	struct net_device *dev = dev_id;
>> >+	struct mpc8xx_irda *si = dev->priv;
>> >+	scc_t *sccp = si->sccp;
>> >+	cbd_t *bdp;
>> >+	ushort int_events;
>> >+	int must_restart = 0;
>> >+
>> >+	/* Get the interrupt events that caused us to be here.
>> >+	 */
>> >+	int_events = in_be16(&sccp->scc_scce);
>> >+	out_be16(&sccp->scc_scce, int_events);
>> >+
>> >+	/* Handle receive event in its own function.
>> >+	 */
>> >+	if (int_events & SCC_AHDLC_RXF)
>> >+		mpc8xx_irda_rx(dev);
>> >+
>> >+	spin_lock(&si->lock);
>> >+
>> >+	/* Transmit OK, or non-fatal error.  Update the buffer
>> >descriptors.
>> >+	 */
>> >+	if (int_events & (SCC_AHDLC_TXE | SCC_AHDLC_TXB)) {
>> >+		bdp = si->dirty_tx;
>> >+
>> >+		while ((bdp->cbd_sc & BD_AHDLC_TX_READY) == 0) {
>> >+			if (si->tx_free == TX_RING_SIZE)
>> >+				break;
>> >+
>> >+			if (bdp->cbd_sc & BD_AHDLC_TX_CTS)
>> >+				must_restart = 1;
>> >+
>> >+			si->stats.tx_packets++;
>> >+
>> >+			/* Free the sk buffer associated with this
>> >last transmit.
>> >+			 */
>> >+
>> >dev_kfree_skb_irq(si->tx_skbuff[si->skb_dirty]);
>> >+			si->skb_dirty = (si->skb_dirty + 1) &
>> >TX_RING_MOD_MASK; +
>> >+			/* Update pointer to next buffer descriptor
>> >to be transmitted.
>> >+			 */
>> >+			if (bdp->cbd_sc & BD_AHDLC_TX_WRAP)
>> >+				bdp = si->tx_bd_base;
>> >+			else
>> >+				bdp++;
>> >+
>> >+			/* Since we have freed up a buffer, the
>> >ring is no longer full.
>> >+			 */
>> >+			if (!si->tx_free++) {
>> >+				if (netif_queue_stopped(dev))
>> >+					netif_wake_queue(dev);
>> >+			}
>> >+
>> >+			si->dirty_tx = (cbd_t *) bdp;
>> >+
>> >+			if (si->newspeed) {
>> >+				mpc8xx_irda_set_speed(dev,
>> >si->newspeed);
>> >+				si->speed = si->newspeed;
>> >+				si->newspeed = 0;
>> >+			}
>> >+		}
>> >+
>> >+		if (must_restart) {
>> >+			cpm8xx_t *cp = immr_map(im_cpm);
>> >+			printk(KERN_INFO "restart TX\n");
>> >+
>> >+			/* Some transmit errors cause the
>> >transmitter to shut
>> >+			 * down.  We now issue a restart transmit.
>> >Since the
>> >+			 * errors close the BD and update the
>> >pointers, the restart
>> >+			 * _should_ pick up without having to reset
>> >any of our
>> >+			 * pointers either.
>> >+			 */
>> >+			out_be16(&cp->cp_cpcr,
>> >+			    mk_cr_cmd(CPM_CR_CH_SCC2,
>> >+				      CPM_CR_RESTART_TX) |
>> >CPM_CR_FLG);
>> >+			while (in_be16(&cp->cp_cpcr) & CPM_CR_FLG) ;
>> You're busy looping in an interrupt handler, that's not really nice.
>That's correct. In fact, I included protection stuff for SCC Ethernet in fs_enet driver to
>get rid of indefinite loop in case something really bad happens in CPM microcode.
>
>I'll move it here.
Good.


>> In general, I think this interrupt handler would deserve a bottom half
>> split since it looks quite busy.
>>
>I don't think it worths bh stuff - scc/FCC/etc ethernet (drivers/net/fs_enet/) use more busy int handler  and is for
>the similar (SCC) SoC device on the same target board (with higher throughout  speed) without performance affected.
>
>It is doable but I guess we'll lose more than win. Also we have to keep in mind, that 8xx PQ SoC is not speed-blasting and having IRQ path between schedule() ticks would not be great either.
>
>So to recap, similar handling path for similar SoC device existing in current kernel for a while now, and working good.

I'm sure it works good, but doing the whole RX path from an interrupt
handler doesn't sound like the best thing to do, at least to me. And
with a SIR IrDA device, you can afford to offload your interrupt
processing to a BH, nobody will notice the difference in terms of
performance.
I'm not against your proposed implementation, but I just think there's
a nicer way to achieve the same result.

>Thanks for review, will follow-up with fixes shortly.
Thanks for the patch.

Cheers,
Samuel.



More information about the Linuxppc-dev mailing list