LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
From: David Miller @ 2009-03-25 21:39 UTC (permalink / raw)
  To: joakim.tjernlund; +Cc: netdev, leoli, linuxppc-dev
In-Reply-To: <1237987849.2194.9.camel@gentoo-jocke.transmode.se>

From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Date: Wed, 25 Mar 2009 14:30:49 +0100

> >From 1c2f23b1f37f4818c0fd0217b93eb38ab6564840 Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Date: Tue, 24 Mar 2009 10:19:27 +0100
> Subject: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
>  Also increase NAPI weight somewhat.
>  This will make the system alot more responsive while
>  ping flooding the ucc_geth ethernet interaface.
> 
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

The weight is not for the sake of your device, it's for
the sake of fairness with others.

Please just use 64, like every other driver does.

I'm not applying this.

^ permalink raw reply

* Re: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
From: David Miller @ 2009-03-25 21:40 UTC (permalink / raw)
  To: dada1; +Cc: leoli, netdev, linuxppc-dev
In-Reply-To: <49CA39EA.6020208@cosmosbay.com>

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 25 Mar 2009 15:04:26 +0100

> Joakim Tjernlund a =E9crit :
> > +	/* Tx event processing */
> > +	spin_lock(&ugeth->lock);
> > +	for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> > +		ucc_geth_tx(ugeth->dev, i);
> > +	}
> > +	spin_unlock(&ugeth->lock);
> > +
> =

> Why tx completions dont change "howmany" ?
> It seems strange you changed UCC_GETH_DEV_WEIGHT if not taking into a=
ccount tx event above...

He should leave howmany alone for TX work and use a weight
value of 64 just like most other drivers in the tree do.

Due to the abuse and random ignorant fiddling of the
weight value, I am going to make it something the core
rather than drivers choose.

^ permalink raw reply

* Re: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
From: David Miller @ 2009-03-25 21:42 UTC (permalink / raw)
  To: Joakim.Tjernlund; +Cc: linuxppc-dev, netdev, leoli, dada1
In-Reply-To: <OF73F7C45F.ACAACBF8-ONC1257584.0052EC21-C1257584.0053E5EF@transmode.se>

From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Wed, 25 Mar 2009 16:16:24 +0100

> UCC_GETH_DEV_WEIGHT needs to be a bit bigger than the number of RX
> HW buffers avaliable, otherwise one won't be able to drain the whole
> queue in one go. Changing weight to something bigger made a big
> difference.

You're not supposed to "drain the whole queue in one go", that
is not the goal of the weight value.

The goal of the weight value is that it is low enough such that
other devices also scheduled for NAPI on the current processor
can get some fair time to process packets too.

^ permalink raw reply

* Re: [PATCH] ucc_geth: Move freeing of TX packets to NAPI context.
From: Joakim Tjernlund @ 2009-03-25 21:55 UTC (permalink / raw)
  To: David Miller; +Cc: linuxppc-dev, netdev, leoli, dada1
In-Reply-To: <20090325.144041.128558998.davem@davemloft.net>

David Miller <davem@davemloft.net> wrote on 25/03/2009 22:40:41:
>=20
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 25 Mar 2009 15:04:26 +0100
>=20
> > Joakim Tjernlund a =E9crit :
> > > +   /* Tx event processing */
> > > +   spin=5Flock(&ugeth->lock);
> > > +   for (i =3D 0; i < ug=5Finfo->numQueuesTx; i++) {
> > > +      ucc=5Fgeth=5Ftx(ugeth->dev, i);
> > > +   }
> > > +   spin=5Funlock(&ugeth->lock);
> > > +
> >=20
> > Why tx completions dont change "howmany" ?
> > It seems strange you changed UCC=5FGETH=5FDEV=5FWEIGHT if not taking in=
to=20
account tx event above...
>=20
> He should leave howmany alone for TX work and use a weight
> value of 64 just like most other drivers in the tree do.

OK, will change this to 64.

>=20
> Due to the abuse and random ignorant fiddling of the
> weight value, I am going to make it something the core
> rather than drivers choose.

Good, I had a hard time finding info how one should use it anyway.=20

^ permalink raw reply

* Re: [PATCH 04/57] microblaze_v7: Open firmware files
From: Benjamin Herrenschmidt @ 2009-03-26  0:01 UTC (permalink / raw)
  To: avorontsov
  Cc: linuxppc-dev, monstr, linux-kernel, Arnd Bergmann, john.williams
In-Reply-To: <20090323205305.GA10576@oksana.dev.rtsoft.ru>

On Mon, 2009-03-23 at 23:53 +0300, Anton Vorontsov wrote:
> It took me approx. 4 hours to factor out the code and make a
> proof-of-concept FDT support for a little-endian ARM platform. ;-)
> (The device tree was only describing a cpu and a nor flash, though.
> No support for interrupt- and gpio-controllers. And no support for
> U-Boot, I just built the dtb file into the kernel).

ARM could benefit tremendously I believe from a similar virtualized
interrupt mapping & OF tree to powerpc for their gazillions of weirdly
cascaded interrupt controller setups :-)

Ben.

^ permalink raw reply

* Re: of_serial and device trees
From: Michael Ellerman @ 2009-03-26  0:53 UTC (permalink / raw)
  To: Scott Wood; +Cc: linuxppc-dev@ozlabs.org, Simon Kagstrom
In-Reply-To: <49CA5E45.9070300@freescale.com>

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

On Wed, 2009-03-25 at 11:39 -0500, Scott Wood wrote:
> Simon Kagstrom wrote:
> > There are no other versions yet, but I suppose there will be (it's
> > implemented in a FPGA after all!). So what is the general handling of
> > versions, should it be something like
> > 
> >   compatible = "ericsson,isf-pic", "ericsson,isf-pic-v2"
> > 
> > etc if we'd make new revisions of the device?
> 
> Looks good.
> 
> >> I'm guessing that your FPGA PIC driver isn't getting its register address
> >> from the device tree, given that it works without the ranges property?
> > 
> > It is, but I didn't check it for correctness yet, so I suppose I might
> > have mapped the wrong thing. The code looks like this:
> > 
> >         struct resource phys_addr;
> > 
> >         if (of_address_to_resource(np, 0, &phys_addr) != 0) {
> >                  printk(KERN_ERR": Could not get ISF PIC memory resource\n");
> >                  return NULL;
> >          }
> > 
> > 	spin_lock_init(&isf->lock);
> > 	isf->ioaddr = ioremap(phys_addr.start,
> > 	                phys_addr.end - phys_addr.start);
> 
> That looks good -- I'd have expected of_address_to_resource to fail, 
> though, when the ranges property was missing.  The kernel's device tree 
> parsing code can sometimes be overly tolerant of broken device trees, 
> which is probably what happened.

It assumes a missing ranges == empty ranges, which means 1:1. See the
comment in of_translate_one(). And thank Apple ;)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Grant Likely @ 2009-03-26  5:09 UTC (permalink / raw)
  To: Wolfgang Grandegger; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <49CA9899.30604@grandegger.com>

On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> For the chip offset, it's not clear what the meaning is. =A0First, does
>> the UPM controller support access of multiple chips simultaneously?
>
> The offset drives the corresponding address lines, which are used to
> select the chip. That's how it's done on the TQM8548 board. In
> principle, the chips could also be selected through dedicated GPIO pins.
> Well, I'm not a hardware guy.

Heh.  I mean elaborate in the binding documentation.  :-)

>> If so, then can you elaborate in the description on how board design
>> translates to a chip-offset value. =A0If it cannot, then it might be
>> better to have multiple tuples in the 'reg' property for each discrete
>> chip. =A0Multiple reg tuples would also remove the need for the
>> num-chips property.
>
> The node still describes one device mapping all relevant control
> registers. How about using fsl,upm-chip-offsets =3D <0x200 0x400>. It
> would be more generic and makes num-chips obsolete as well. And the
> property would be reserved for that way of implementing the chip select
> in hardware.

It really sounds like this binding is describing multiple NAND chips
mapped to different base addresses (and looking at the fsm_upm.c
driver appears to confirm it).  So, does this work?  reg =3D <3 0x200 4
 3 0x400 4>;

It is true that other methods could be used for implementing the chip
select, but that is *not* what the proposed binding describes.  This
proposed binding describes NAND chips selected by address lines
(particular addresses), and in this case I think using reg is the
natural description.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH] powerpc: Sanitize stack pointer in signal handling code
From: Benjamin Herrenschmidt @ 2009-03-26  5:59 UTC (permalink / raw)
  To: Josh Boyer; +Cc: linuxppc-dev
In-Reply-To: <20090325162700.GB2367@yoda.jdub.homelinux.org>


> >Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
> 
> Ben, this should look familiar.  It's based on your current -next branch.
> 
> If you agree, I can send it to the -stable team for .27, .28 and .29.

Patch looks good. I'll review more closely tomorrow morning and put it
in my -next branch, which I'll then ask Linux to pull. At this point, we
can have it backported into stable.

Cheers,
Ben.

^ permalink raw reply

* Re: Next March 25: Boot failure on powerpc [recursive locking detected]
From: Sachin Sant @ 2009-03-26  6:34 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: James Bottomley, Stephen Rothwell, linux-next, linux-scsi
In-Reply-To: <49CA3BF4.3050801@in.ibm.com>

Sachin Sant wrote:
> Today's next failed to boot on a powerpc box
> (Power6 blade IBM,7998-61X) with following recursive locking message.
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 2.6.29-next-20090325 #1
After bisecting the failure seems to be because of the following
patch from James ( block: move SCSI timeout check into block )

http://patchwork.kernel.org/patch/8017/

If i back out the above mentioned patch, the machine boots fine
without any problems.

Thanks
-Sachin


-- 

---------------------------------
Sachin Sant
IBM Linux Technology Center
India Systems and Technology Labs
Bangalore, India
---------------------------------

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Wolfgang Grandegger @ 2009-03-26  7:42 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <fa686aa40903252209r52a1bc1cn995a7da16bc3527f@mail.gmail.com>

Grant Likely wrote:
> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> For the chip offset, it's not clear what the meaning is.  First, does
>>> the UPM controller support access of multiple chips simultaneously?
>> The offset drives the corresponding address lines, which are used to
>> select the chip. That's how it's done on the TQM8548 board. In
>> principle, the chips could also be selected through dedicated GPIO pins.
>> Well, I'm not a hardware guy.
> 
> Heh.  I mean elaborate in the binding documentation.  :-)
> 
>>> If so, then can you elaborate in the description on how board design
>>> translates to a chip-offset value.  If it cannot, then it might be
>>> better to have multiple tuples in the 'reg' property for each discrete
>>> chip.  Multiple reg tuples would also remove the need for the
>>> num-chips property.
>> The node still describes one device mapping all relevant control
>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>> would be more generic and makes num-chips obsolete as well. And the
>> property would be reserved for that way of implementing the chip select
>> in hardware.
> 
> It really sounds like this binding is describing multiple NAND chips
> mapped to different base addresses (and looking at the fsm_upm.c
> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>  3 0x400 4>;

The chip-offset, and not the address, needs to be added to the MAR
register as well before running the pattern:

        mar = cmd << (32 - fun->upm.width);

        if (fun->chip_offset && fun->chip_number > 0)

                mar |= fun->chip_number * fun->chip_offset;

        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);


> It is true that other methods could be used for implementing the chip
> select, but that is *not* what the proposed binding describes.  This
> proposed binding describes NAND chips selected by address lines
> (particular addresses), and in this case I think using reg is the
> natural description.

OK, the chips are selected by accessing a defined address range. Will
prepare a patch using the reg property.

Wolfgang.




> g.
> 

^ permalink raw reply

* [PATCH v2] Fix RTAS watchdog driver temperature read functionality
From: Adrian Reber @ 2009-03-26 12:05 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Utz Bacher
In-Reply-To: <1237928433.25062.725.camel@pasglop>

Using the RTAS watchdog driver to read out the temperature crashes
on a PXCAB:

Unable to handle kernel paging request for data at address 0xfe347b50
Faulting instruction address: 0xc00000000001af64
Oops: Kernel access of bad area, sig: 11 [#1]

The wrong usage of "(void *)__pa(&temperature)" in rtas_call() is
removed by using the function rtas_get_sensor() which does the
right thing.

Signed-off-by: Adrian Reber <adrian@lisas.de>
---
v2: use rtas_get_sensor(); typo fixed
---
 drivers/watchdog/wdrtas.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/wdrtas.c b/drivers/watchdog/wdrtas.c
index 5d3b1a8..a9f00dc 100644
--- a/drivers/watchdog/wdrtas.c
+++ b/drivers/watchdog/wdrtas.c
@@ -214,16 +214,14 @@ static void wdrtas_timer_keepalive(void)
  */
 static int wdrtas_get_temperature(void)
 {
-	long result;
+	int result;
 	int temperature = 0;
 
-	result = rtas_call(wdrtas_token_get_sensor_state, 2, 2,
-			   (void *)__pa(&temperature),
-			   WDRTAS_THERMAL_SENSOR, 0);
+	result = rtas_get_sensor(WDRTAS_THERMAL_SENSOR, 0, &temperature);
 
 	if (result < 0)
 		printk(KERN_WARNING "wdrtas: reading the thermal sensor "
-		       "faild: %li\n", result);
+		       "failed: %i\n", result);
 	else
 		temperature = ((temperature * 9) / 5) + 32; /* fahrenheit */
 
-- 
1.5.6.6

^ permalink raw reply related

* [PATCH 2/2] ucc_geth: Rework the TX logic.
From: Joakim Tjernlund @ 2009-03-26 12:54 UTC (permalink / raw)
  To: avorontsov, leoli, linuxppc-dev, netdev; +Cc: Joakim Tjernlund
In-Reply-To: <1238072077-27044-1-git-send-email-Joakim.Tjernlund@transmode.se>

The line:
 if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
       break;
in ucc_geth_tx() didn not make sense to me. Rework & cleanup
this logic to something understandable.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/net/ucc_geth.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 7fc91aa..b6ebefd 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u8 __iomem *bd;			/* BD pointer */
 	u32 bd_status;
 	u8 txQ = 0;
+	int txInd;
 
 	ugeth_vdbg("%s: IN", __func__);
 
@@ -3059,12 +3060,12 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	bd = ugeth->txBd[txQ];
 	bd_status = in_be32((u32 __iomem *)bd);
 	/* Save the skb pointer so we can free it later */
-	ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]] = skb;
+	txInd = ugeth->skb_curtx[txQ];
+	ugeth->tx_skbuff[txQ][txInd] = skb;
 
 	/* Update the current skb pointer (wrapping if this was the last) */
-	ugeth->skb_curtx[txQ] =
-	    (ugeth->skb_curtx[txQ] +
-	     1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
+	txInd = (txInd + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
+	ugeth->skb_curtx[txQ] = txInd;
 
 	/* set up the buffer descriptor */
 	out_be32(&((struct qe_bd __iomem *)bd)->buf,
@@ -3088,9 +3089,8 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* If the next BD still needs to be cleaned up, then the bds
 	   are full.  We need to tell the kernel to stop sending us stuff. */
-	if (bd == ugeth->confBd[txQ]) {
-		if (!netif_queue_stopped(dev))
-			netif_stop_queue(dev);
+	if (!in_be32((u32 __iomem *)(bd+4))) {
+		netif_stop_queue(dev);
 	}
 
 	ugeth->txBd[txQ] = bd;
@@ -3198,32 +3198,29 @@ static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 	struct ucc_geth_private *ugeth = netdev_priv(dev);
 	u8 __iomem *bd;		/* BD pointer */
 	u32 bd_status;
+	int txInd, num_freed;
 
 	bd = ugeth->confBd[txQ];
 	bd_status = in_be32((u32 __iomem *)bd);
-
+	txInd = ugeth->skb_dirtytx[txQ];
+	num_freed = 0;
 	/* Normal processing. */
 	while ((bd_status & T_R) == 0) {
 		/* BD contains already transmitted buffer.   */
 		/* Handle the transmitted buffer and release */
 		/* the BD to be used with the current frame  */
 
-		if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
-			break;
+		if (!in_be32((u32 __iomem *)(bd+4)))
+			break; /* Queue is empty */
 
 		dev->stats.tx_packets++;
 
 		/* Free the sk buffer associated with this TxBD */
-		dev_kfree_skb(ugeth->
-				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
-		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
-		ugeth->skb_dirtytx[txQ] =
-		    (ugeth->skb_dirtytx[txQ] +
-		     1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
-
-		/* We freed a buffer, so now we can restart transmission */
-		if (netif_queue_stopped(dev))
-			netif_wake_queue(dev);
+		dev_kfree_skb(ugeth->tx_skbuff[txQ][txInd]);
+		ugeth->tx_skbuff[txQ][txInd] = NULL;
+		out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */
+		num_freed++;
+		txInd = (txInd + 1) & TX_RING_MOD_MASK(ugeth->ug_info->bdRingLenTx[txQ]);
 
 		/* Advance the confirmation BD pointer */
 		if (!(bd_status & T_W))
@@ -3233,6 +3230,9 @@ static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 		bd_status = in_be32((u32 __iomem *)bd);
 	}
 	ugeth->confBd[txQ] = bd;
+	ugeth->skb_dirtytx[txQ] = txInd;
+	if (num_freed)
+		netif_wake_queue(dev); /* We freed some buffers, so restart transmission */
 	return 0;
 }
 
-- 
1.6.1.3

^ permalink raw reply related

* [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.
From: Joakim Tjernlund @ 2009-03-26 12:54 UTC (permalink / raw)
  To: avorontsov, leoli, linuxppc-dev, netdev; +Cc: Joakim Tjernlund

Also set NAPI weight to 64 as this is a common value.
This will make the system alot more responsive while
ping flooding the ucc_geth ethernet interaface.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 drivers/net/ucc_geth.c |   32 ++++++++++++--------------------
 drivers/net/ucc_geth.h |    1 -
 2 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 097aed8..7fc91aa 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -3214,7 +3214,7 @@ static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 		dev->stats.tx_packets++;
 
 		/* Free the sk buffer associated with this TxBD */
-		dev_kfree_skb_irq(ugeth->
+		dev_kfree_skb(ugeth->
 				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
 		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
 		ugeth->skb_dirtytx[txQ] =
@@ -3248,9 +3248,16 @@ static int ucc_geth_poll(struct napi_struct *napi, int budget)
 	for (i = 0; i < ug_info->numQueuesRx; i++)
 		howmany += ucc_geth_rx(ugeth, i, budget - howmany);
 
+	/* Tx event processing */
+	spin_lock(&ugeth->lock);
+	for (i = 0; i < ug_info->numQueuesTx; i++) {
+		ucc_geth_tx(ugeth->dev, i);
+	}
+	spin_unlock(&ugeth->lock);
+
 	if (howmany < budget) {
 		netif_rx_complete(napi);
-		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS);
+		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 	}
 
 	return howmany;
@@ -3264,8 +3271,6 @@ static irqreturn_t ucc_geth_irq_handler(int irq, void *info)
 	struct ucc_geth_info *ug_info;
 	register u32 ucce;
 	register u32 uccm;
-	register u32 tx_mask;
-	u8 i;
 
 	ugeth_vdbg("%s: IN", __func__);
 
@@ -3279,27 +3284,14 @@ static irqreturn_t ucc_geth_irq_handler(int irq, void *info)
 	out_be32(uccf->p_ucce, ucce);
 
 	/* check for receive events that require processing */
-	if (ucce & UCCE_RX_EVENTS) {
+	if (ucce & (UCCE_RX_EVENTS | UCCE_TX_EVENTS)) {
 		if (netif_rx_schedule_prep(&ugeth->napi)) {
-			uccm &= ~UCCE_RX_EVENTS;
+			uccm &= ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
 			out_be32(uccf->p_uccm, uccm);
 			__netif_rx_schedule(&ugeth->napi);
 		}
 	}
 
-	/* Tx event processing */
-	if (ucce & UCCE_TX_EVENTS) {
-		spin_lock(&ugeth->lock);
-		tx_mask = UCC_GETH_UCCE_TXB0;
-		for (i = 0; i < ug_info->numQueuesTx; i++) {
-			if (ucce & tx_mask)
-				ucc_geth_tx(dev, i);
-			ucce &= ~tx_mask;
-			tx_mask <<= 1;
-		}
-		spin_unlock(&ugeth->lock);
-	}
-
 	/* Errors and other events */
 	if (ucce & UCCE_OTHER) {
 		if (ucce & UCC_GETH_UCCE_BSY)
@@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 	dev->netdev_ops = &ucc_geth_netdev_ops;
 	dev->watchdog_timeo = TX_TIMEOUT;
 	INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
-	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, UCC_GETH_DEV_WEIGHT);
+	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
 	dev->mtu = 1500;
 
 	ugeth->msg_enable = netif_msg_init(debug.msg_enable, UGETH_MSG_DEFAULT);
diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
index 44218b8..50bad53 100644
--- a/drivers/net/ucc_geth.h
+++ b/drivers/net/ucc_geth.h
@@ -843,7 +843,6 @@ struct ucc_geth_hardware_statistics {
 /* Driver definitions */
 #define TX_BD_RING_LEN                          0x10
 #define RX_BD_RING_LEN                          0x10
-#define UCC_GETH_DEV_WEIGHT                     TX_BD_RING_LEN
 
 #define TX_RING_MOD_MASK(size)                  (size-1)
 #define RX_RING_MOD_MASK(size)                  (size-1)
-- 
1.6.1.3

^ permalink raw reply related

* Re: [PATCH 2/2] ucc_geth: Rework the TX logic.
From: Anton Vorontsov @ 2009-03-26 13:39 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev, leoli, netdev
In-Reply-To: <1238072077-27044-2-git-send-email-Joakim.Tjernlund@transmode.se>

Hi Joakim,

On Thu, Mar 26, 2009 at 01:54:37PM +0100, Joakim Tjernlund wrote:
> The line:
>  if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
>        break;
> in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> this logic to something understandable.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/net/ucc_geth.c |   40 ++++++++++++++++++++--------------------
>  1 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 7fc91aa..b6ebefd 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	u8 __iomem *bd;			/* BD pointer */
>  	u32 bd_status;
>  	u8 txQ = 0;
> +	int txInd;

camelCase should not be used in Linux.

Surely, the driver is full of camelCases... though, it should be
fixed, not encouraged further.

And btw, there is even Hungarian notation in the driver. :-(

[...]
>  	/* If the next BD still needs to be cleaned up, then the bds
>  	   are full.  We need to tell the kernel to stop sending us stuff. */
> -	if (bd == ugeth->confBd[txQ]) {
> -		if (!netif_queue_stopped(dev))
> -			netif_stop_queue(dev);
> +	if (!in_be32((u32 __iomem *)(bd+4))) {

bd == ugeth->confBd[txQ]
and
!in_be32((u32 __iomem *)(bd+4))

Are not equivalent wrt. speed. MMIO accessors should be rather
slow comparing to normal memory.

We should really do some performance tests (using gbit links).
I'll try to help you with the tests, but it might take some time.

[...]
> +		if (!in_be32((u32 __iomem *)(bd+4)))
[...]
> +		out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */

How about some inline function that will self-document the bd + 4
stuff? Plus that way we'll get rid of the casts.

Note that "bd+4" should be "bd + 4". And (int)NULL makes
little sense, just 0 will work.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

^ permalink raw reply

* Re: sata device failed to IDENTIFY...
From: rizwan ahmad @ 2009-03-26 13:56 UTC (permalink / raw)
  To: linuxppc-dev
In-Reply-To: <22655709.post@talk.nabble.com>




output of lspci -vv

-bash-3.2# lspci -vv
00:0c.0 Class 0c03: Unknown device 1106:3038 (rev 62)
        Subsystem: Unknown device 1106:3038
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Ste-        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
<TAbor-        Latency: 128
        Interrupt: pin A routed to IRQ 25
        Region 4: I/O ports at ffe0 [size=32]
        Capabilities: [80] Power Management version 2
                Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA
PME(D0+,D1+,D2+,D3)                Status: D0 PME-Enable- DSel=0 DScale=0
PME-

00:0c.1 Class 0c03: Unknown device 1106:3038 (rev 62)
        Subsystem: Unknown device 1106:3038
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Ste-        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
<TAbor-        Latency: 128
        Interrupt: pin B routed to IRQ 25
        Region 4: I/O ports at ffc0 [size=32]
        Capabilities: [80] Power Management version 2
                Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA
PME(D0+,D1+,D2+,D3)                Status: D0 PME-Enable- DSel=0 DScale=0
PME-

00:0c.2 Class 0c03: Unknown device 1106:3104 (rev 65) (prog-if 20)
        Subsystem: Unknown device 1106:3104
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Ste-        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
<TAbor-        Latency: 128
        Interrupt: pin C routed to IRQ 25
        Region 0: Memory at afffff00 (32-bit, non-prefetchable) [size=256]
        Capabilities: [80] Power Management version 2
                Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA
PME(D0+,D1+,D2+,D3)                Status: D0 PME-Enable- DSel=0 DScale=0
PME-

00:0c.3 Class 0104: Unknown device 1106:3249 (rev 50)
        Subsystem: Unknown device 1106:3249
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Ste-        Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
<TAbor-        Latency: 128
        Interrupt: pin A routed to IRQ 25
        Region 0: I/O ports at ffb0 [size=16]
        Region 1: I/O ports at ffa0 [size=16]
        Region 2: I/O ports at ff90 [size=16]
        Region 3: I/O ports at ff80 [size=16]
        Region 4: I/O ports at ff60 [size=32]
        Region 5: I/O ports at fe00 [size=256]
        Expansion ROM at <ignored> [disabled]
        Capabilities: [e0] Power Management version 2
                Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
PME(D0-,D1-,D2-,D3ho)                Status: D0 PME-Enable- DSel=0 DScale=0
PME-

-bash-3.2#



-- 
View this message in context: http://www.nabble.com/sata-device-failed-to-IDENTIFY...-tp22655709p22722574.html
Sent from the linuxppc-dev mailing list archive at Nabble.com.

^ permalink raw reply

* Re: Next March 25: Boot failure on powerpc [recursive locking detected]
From: James Bottomley @ 2009-03-26 13:50 UTC (permalink / raw)
  To: Sachin Sant; +Cc: linuxppc-dev, linux-next, linux-scsi, Stephen Rothwell
In-Reply-To: <49CB2200.50703@in.ibm.com>

On Thu, 2009-03-26 at 12:04 +0530, Sachin Sant wrote:
> Sachin Sant wrote:
> > Today's next failed to boot on a powerpc box
> > (Power6 blade IBM,7998-61X) with following recursive locking message.
> >
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 2.6.29-next-20090325 #1
> After bisecting the failure seems to be because of the following
> patch from James ( block: move SCSI timeout check into block )
> 
> http://patchwork.kernel.org/patch/8017/
> 
> If i back out the above mentioned patch, the machine boots fine
> without any problems.

Yes, that patch already got dropped for other reasons:

http://marc.info/?t=123740773700002

I'm going to see if I can redo it in a better way, since moving this
type of timeout checking from scsi to block is a useful generalisation.

James

^ permalink raw reply

* Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.
From: Eric Dumazet @ 2009-03-26 14:15 UTC (permalink / raw)
  To: Joakim Tjernlund; +Cc: linuxppc-dev, leoli, netdev
In-Reply-To: <1238072077-27044-1-git-send-email-Joakim.Tjernlund@transmode.se>

Joakim Tjernlund a =E9crit :
> Also set NAPI weight to 64 as this is a common value.
> This will make the system alot more responsive while
> ping flooding the ucc_geth ethernet interaface.
>=20
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/net/ucc_geth.c |   32 ++++++++++++--------------------
>  drivers/net/ucc_geth.h |    1 -
>  2 files changed, 12 insertions(+), 21 deletions(-)
>=20
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 097aed8..7fc91aa 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -3214,7 +3214,7 @@ static int ucc_geth_tx(struct net_device *dev, u8=
 txQ)
>  		dev->stats.tx_packets++;
> =20
>  		/* Free the sk buffer associated with this TxBD */
> -		dev_kfree_skb_irq(ugeth->
> +		dev_kfree_skb(ugeth->
>  				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
>  		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] =3D NULL;
>  		ugeth->skb_dirtytx[txQ] =3D
> @@ -3248,9 +3248,16 @@ static int ucc_geth_poll(struct napi_struct *nap=
i, int budget)

>  	for (i =3D 0; i < ug_info->numQueuesRx; i++)
>  		howmany +=3D ucc_geth_rx(ugeth, i, budget - howmany);

Not related to your patch, but seeing above code, I can understand
you might have a problem in flood situation : Only first queue(s) are
depleted, and last one cannot be because howmany >=3D budget.

This driver might have to remember last queue it handled at previous
call ucc_geth_poll(), so that all rxqueues have same probability to be sc=
anned.

j =3D ug->lastslot;
for (i =3D 0; ug_info->numQueuesRx; i++) {
	if (++j >=3D ug_info->numQueuesRx)
		j =3D 0;
	howmany +=3D ucc_geth_rx(ugeth, j, budget - howmany);
	if (howmany >=3D budget)
		break;
}
ug->lastslot =3D j;


> =20
> +	/* Tx event processing */
> +	spin_lock(&ugeth->lock);
> +	for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> +		ucc_geth_tx(ugeth->dev, i);
> +	}
> +	spin_unlock(&ugeth->lock);
> +
>  	if (howmany < budget) {
>  		netif_rx_complete(napi);
> -		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS);
> +		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
>  	}
> =20
>  	return howmany;
> @@ -3264,8 +3271,6 @@ static irqreturn_t ucc_geth_irq_handler(int irq, =
void *info)
>  	struct ucc_geth_info *ug_info;
>  	register u32 ucce;
>  	register u32 uccm;
> -	register u32 tx_mask;
> -	u8 i;
> =20
>  	ugeth_vdbg("%s: IN", __func__);
> =20
> @@ -3279,27 +3284,14 @@ static irqreturn_t ucc_geth_irq_handler(int irq=
, void *info)
>  	out_be32(uccf->p_ucce, ucce);
> =20
>  	/* check for receive events that require processing */
> -	if (ucce & UCCE_RX_EVENTS) {
> +	if (ucce & (UCCE_RX_EVENTS | UCCE_TX_EVENTS)) {
>  		if (netif_rx_schedule_prep(&ugeth->napi)) {
> -			uccm &=3D ~UCCE_RX_EVENTS;
> +			uccm &=3D ~(UCCE_RX_EVENTS | UCCE_TX_EVENTS);
>  			out_be32(uccf->p_uccm, uccm);
>  			__netif_rx_schedule(&ugeth->napi);
>  		}
>  	}
> =20
> -	/* Tx event processing */
> -	if (ucce & UCCE_TX_EVENTS) {
> -		spin_lock(&ugeth->lock);
> -		tx_mask =3D UCC_GETH_UCCE_TXB0;
> -		for (i =3D 0; i < ug_info->numQueuesTx; i++) {
> -			if (ucce & tx_mask)
> -				ucc_geth_tx(dev, i);
> -			ucce &=3D ~tx_mask;
> -			tx_mask <<=3D 1;
> -		}
> -		spin_unlock(&ugeth->lock);
> -	}
> -
>  	/* Errors and other events */
>  	if (ucce & UCCE_OTHER) {
>  		if (ucce & UCC_GETH_UCCE_BSY)
> @@ -3733,7 +3725,7 @@ static int ucc_geth_probe(struct of_device* ofdev=
, const struct of_device_id *ma
>  	dev->netdev_ops =3D &ucc_geth_netdev_ops;
>  	dev->watchdog_timeo =3D TX_TIMEOUT;
>  	INIT_WORK(&ugeth->timeout_work, ucc_geth_timeout_work);
> -	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, UCC_GETH_DEV_WEIGHT)=
;
> +	netif_napi_add(dev, &ugeth->napi, ucc_geth_poll, 64);
>  	dev->mtu =3D 1500;
> =20
>  	ugeth->msg_enable =3D netif_msg_init(debug.msg_enable, UGETH_MSG_DEFA=
ULT);
> diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
> index 44218b8..50bad53 100644
> --- a/drivers/net/ucc_geth.h
> +++ b/drivers/net/ucc_geth.h
> @@ -843,7 +843,6 @@ struct ucc_geth_hardware_statistics {
>  /* Driver definitions */
>  #define TX_BD_RING_LEN                          0x10
>  #define RX_BD_RING_LEN                          0x10
> -#define UCC_GETH_DEV_WEIGHT                     TX_BD_RING_LEN
> =20
>  #define TX_RING_MOD_MASK(size)                  (size-1)
>  #define RX_RING_MOD_MASK(size)                  (size-1)

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Grant Likely @ 2009-03-26 14:27 UTC (permalink / raw)
  To: Wolfgang Grandegger; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <49CB31CB.2010704@grandegger.com>

On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com>=
 wrote:
>>> Grant Likely wrote:
>>>> For the chip offset, it's not clear what the meaning is. =A0First, doe=
s
>>>> the UPM controller support access of multiple chips simultaneously?
>>> The offset drives the corresponding address lines, which are used to
>>> select the chip. That's how it's done on the TQM8548 board. In
>>> principle, the chips could also be selected through dedicated GPIO pins=
.
>>> Well, I'm not a hardware guy.
>>
>> Heh. =A0I mean elaborate in the binding documentation. =A0:-)
>>
>>>> If so, then can you elaborate in the description on how board design
>>>> translates to a chip-offset value. =A0If it cannot, then it might be
>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>> chip. =A0Multiple reg tuples would also remove the need for the
>>>> num-chips property.
>>> The node still describes one device mapping all relevant control
>>> registers. How about using fsl,upm-chip-offsets =3D <0x200 0x400>. It
>>> would be more generic and makes num-chips obsolete as well. And the
>>> property would be reserved for that way of implementing the chip select
>>> in hardware.
>>
>> It really sounds like this binding is describing multiple NAND chips
>> mapped to different base addresses (and looking at the fsm_upm.c
>> driver appears to confirm it). =A0So, does this work? =A0reg =3D <3 0x20=
0 4
>> =A03 0x400 4>;
>
> The chip-offset, and not the address, needs to be added to the MAR
> register as well before running the pattern:
>
> =A0 =A0 =A0 =A0mar =3D cmd << (32 - fun->upm.width);
>
> =A0 =A0 =A0 =A0if (fun->chip_offset && fun->chip_number > 0)
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mar |=3D fun->chip_number * fun->chip_offs=
et;
>
> =A0 =A0 =A0 =A0fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>
>
>> It is true that other methods could be used for implementing the chip
>> select, but that is *not* what the proposed binding describes. =A0This
>> proposed binding describes NAND chips selected by address lines
>> (particular addresses), and in this case I think using reg is the
>> natural description.
>
> OK, the chips are selected by accessing a defined address range. Will
> prepare a patch using the reg property.

Hold on a sec.  I'm debating from my experience with device tree
bindings, but I'm fairly ignorant about the implementation of NAND on
UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
tell me so and why.  Your comment above about fsl_upm_run_pattern()
makes me doubt my position.

Does using the reg property give the driver enough information to
reliably program the MAR for NAND connections that use the address
line chip select scheme?  Related to that, should the binding include
a property that explicitly states that an address line chip select
scheme is being used?

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH v3] powerpc: clean up ssi.txt, add definition for fsl, ssi-asynchronous
From: Timur Tabi @ 2009-03-26 15:14 UTC (permalink / raw)
  To: linuxppc-dev, galak
In-Reply-To: <1236817324-24612-1-git-send-email-timur@freescale.com>

On Wed, Mar 11, 2009 at 7:22 PM, Timur Tabi <timur@freescale.com> wrote:
> Add the definition of the fsl,ssi-asynchronous property to ssi.txt (documentation
> of the device tree bindings for the Freescale SSI device).
>
> Also tidy up the layout of ssi.txt.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>

Kumar, can you pick this up for 2.6.30?

-- 
Timur Tabi
Linux kernel developer at Freescale

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Wolfgang Grandegger @ 2009-03-26 15:33 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <fa686aa40903260727y3266e394g5e574680fe70bbbf@mail.gmail.com>

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>>> Grant Likely wrote:
>>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>>> the UPM controller support access of multiple chips simultaneously?
>>>> The offset drives the corresponding address lines, which are used to
>>>> select the chip. That's how it's done on the TQM8548 board. In
>>>> principle, the chips could also be selected through dedicated GPIO pins.
>>>> Well, I'm not a hardware guy.
>>> Heh.  I mean elaborate in the binding documentation.  :-)
>>>
>>>>> If so, then can you elaborate in the description on how board design
>>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>>> chip.  Multiple reg tuples would also remove the need for the
>>>>> num-chips property.
>>>> The node still describes one device mapping all relevant control
>>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>>> would be more generic and makes num-chips obsolete as well. And the
>>>> property would be reserved for that way of implementing the chip select
>>>> in hardware.
>>> It really sounds like this binding is describing multiple NAND chips
>>> mapped to different base addresses (and looking at the fsm_upm.c
>>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>>  3 0x400 4>;
>> The chip-offset, and not the address, needs to be added to the MAR
>> register as well before running the pattern:
>>
>>        mar = cmd << (32 - fun->upm.width);
>>
>>        if (fun->chip_offset && fun->chip_number > 0)
>>
>>                mar |= fun->chip_number * fun->chip_offset;
>>
>>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>>
>>> It is true that other methods could be used for implementing the chip
>>> select, but that is *not* what the proposed binding describes.  This
>>> proposed binding describes NAND chips selected by address lines
>>> (particular addresses), and in this case I think using reg is the
>>> natural description.
>> OK, the chips are selected by accessing a defined address range. Will
>> prepare a patch using the reg property.
> 
> Hold on a sec.  I'm debating from my experience with device tree

I already started ;-).

> bindings, but I'm fairly ignorant about the implementation of NAND on
> UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
> tell me so and why.  Your comment above about fsl_upm_run_pattern()
> makes me doubt my position.

It's not sufficient to just map the related space and access it, at least.

> Does using the reg property give the driver enough information to
> reliably program the MAR for NAND connections that use the address
> line chip select scheme?  Related to that, should the binding include

In principle yes:

  if (i > 0)
      offset[i] = resource[i].start - resource[0].start;

> a property that explicitly states that an address line chip select
> scheme is being used?

That's why I'm still in favor of:

  fsl,upm-multi-chip-offsets = <0x200 0x400>

That would state that the address line chip select scheme is used with
the specified offsets. It also allows for a more elegant solution
(code-wise).

Wolfgang.

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Grant Likely @ 2009-03-26 16:04 UTC (permalink / raw)
  To: Wolfgang Grandegger; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <49CBA062.5050000@grandegger.com>

On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> Does using the reg property give the driver enough information to
>> reliably program the MAR for NAND connections that use the address
>> line chip select scheme? =A0Related to that, should the binding include
>
> In principle yes:
>
> =A0if (i > 0)
> =A0 =A0 =A0offset[i] =3D resource[i].start - resource[0].start;

Ewww.  That's ugly.

>> a property that explicitly states that an address line chip select
>> scheme is being used?
>
> That's why I'm still in favor of:
>
> =A0fsl,upm-multi-chip-offsets =3D <0x200 0x400>
>
> That would state that the address line chip select scheme is used with
> the specified offsets. It also allows for a more elegant solution
> (code-wise).

Alright.  Then at the very least the property name should reflect that
address lines CS is used to reduce the chance of confusion with
another multi-chip scheme.  Something like
fsl,upm-addr-line-cs-offsets maybe?

Here is another thought.  The binding is describing that address lines
are used to activate CS lines.  Offset for chip access purposes is
derived from the address line, but it doesn't directly describe the
hardware.  The following may be a better description of the hardware.

fsl,upm-addr-line-cs =3D <9 10>;

g.



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: ELDK with Xenomai for PPC
From: Scott Wood @ 2009-03-26 16:11 UTC (permalink / raw)
  To: Goldberger, Yigal; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <7913DB5FA7D3F643A2C10A4A8E2DFAF43A3A5D7C25@ILMA1.IL.NDS.COM>

On Wed, Mar 25, 2009 at 07:06:50PM +0200, Goldberger, Yigal wrote:
> I'll try to be as accurate as I can.
> Yesterday I downloaded ELDK 4.2 from DENX site and Installed it.
> I want to build a Xenomai enabled kernel.
> I saw that my kernel is 2.6.24-xenomai and that's what I'm trying to build.
[snip]
> Currently I did :
> > make ads8272_defconfig
> > make uImage
> 
> and I'm failing on :
>   CC      drivers/net/fs_enet/mac-fcc.o
> drivers/net/fs_enet/mac-fcc.c: In function 'fcc_cr_cmd':
> drivers/net/fs_enet/mac-fcc.c:85: error: implicit declaration of function 'cpm_command'
> make[3]: *** [drivers/net/fs_enet/mac-fcc.o] Error 1
> make[2]: *** [drivers/net/fs_enet] Error 2
> make[1]: *** [drivers/net] Error 2
> make: *** [drivers] Error 2

Please try a more recent kernel; there was a lot of cleanup of 82xx
support going on in the 2.6.24 timeframe.  Also, in general you should
try building a vanilla kernel of the same version/platform, to see if it
is a problem introduced by xenomai.  Note that in vanilla 2.6.24 I do not
see any reference to cpm_command -- it looks like you have the 2.6.25
version of mac-fcc.c.

Is this arch/powerpc or arch/ppc?  If the latter, it's no longer
supported, is gone in current kernels, and was highly deprecated back
when 2.6.24 came out.  It seems that the last two releases with arch/ppc
(2.6.25 and 2.6.26) had this problem.

-Scott

^ permalink raw reply

* Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
From: Wolfgang Grandegger @ 2009-03-26 16:35 UTC (permalink / raw)
  To: Grant Likely; +Cc: linuxppc-dev, devicetree-discuss list, linux-mtd
In-Reply-To: <fa686aa40903260904s67258fbbge3c87e06946c5f58@mail.gmail.com>

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> Does using the reg property give the driver enough information to
>>> reliably program the MAR for NAND connections that use the address
>>> line chip select scheme?  Related to that, should the binding include
>> In principle yes:
>>
>>  if (i > 0)
>>      offset[i] = resource[i].start - resource[0].start;
> 
> Ewww.  That's ugly.

Yep.

>>> a property that explicitly states that an address line chip select
>>> scheme is being used?
>> That's why I'm still in favor of:
>>
>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>
>> That would state that the address line chip select scheme is used with
>> the specified offsets. It also allows for a more elegant solution
>> (code-wise).
> 
> Alright.  Then at the very least the property name should reflect that
> address lines CS is used to reduce the chance of confusion with
> another multi-chip scheme.  Something like
> fsl,upm-addr-line-cs-offsets maybe?


> 
> Here is another thought.  The binding is describing that address lines
> are used to activate CS lines.  Offset for chip access purposes is
> derived from the address line, but it doesn't directly describe the
> hardware.  The following may be a better description of the hardware.
> 
> fsl,upm-addr-line-cs = <9 10>;

The TQM8548 hardware has some logic connected to the two address lines
allowing to select up to 4 chips with two address lines:

 fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

That's the more general solution.

Wolfgang.

^ permalink raw reply

* Re: [PATCH 2/2] ucc_geth: Rework the TX logic.
From: Joakim Tjernlund @ 2009-03-26 16:43 UTC (permalink / raw)
  To: avorontsov; +Cc: linuxppc-dev, leoli, netdev
In-Reply-To: <20090326133918.GA27085@oksana.dev.rtsoft.ru>

Anton Vorontsov <avorontsov@ru.mvista.com> wrote on 26/03/2009 14:39:18:
> 
> Hi Joakim,

Hi Anton

> 
> On Thu, Mar 26, 2009 at 01:54:37PM +0100, Joakim Tjernlund wrote:
> > The line:
> >  if ((bd == ugeth->txBd[txQ]) && (netif_queue_stopped(dev) == 0))
> >        break;
> > in ucc_geth_tx() didn not make sense to me. Rework & cleanup
> > this logic to something understandable.
> > 
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  drivers/net/ucc_geth.c |   40 
++++++++++++++++++++--------------------
> >  1 files changed, 20 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> > index 7fc91aa..b6ebefd 100644
> > --- a/drivers/net/ucc_geth.c
> > +++ b/drivers/net/ucc_geth.c
> > @@ -3048,6 +3048,7 @@ static int ucc_geth_start_xmit(struct sk_buff 
*skb, struct net_device *dev)
> >     u8 __iomem *bd;         /* BD pointer */
> >     u32 bd_status;
> >     u8 txQ = 0;
> > +   int txInd;
> 
> camelCase should not be used in Linux.
> 
> Surely, the driver is full of camelCases... though, it should be
> fixed, not encouraged further.

OK, I will rename to tx_ind instead.

> 
> And btw, there is even Hungarian notation in the driver. :-(

Hopefully that will go away in time.

> 
> [...]
> >     /* If the next BD still needs to be cleaned up, then the bds
> >        are full.  We need to tell the kernel to stop sending us stuff. 
*/
> > -   if (bd == ugeth->confBd[txQ]) {
> > -      if (!netif_queue_stopped(dev))
> > -         netif_stop_queue(dev);
> > +   if (!in_be32((u32 __iomem *)(bd+4))) {
> 
> bd == ugeth->confBd[txQ]
> and
> !in_be32((u32 __iomem *)(bd+4))
> 
> Are not equivalent wrt. speed. MMIO accessors should be rather
> slow comparing to normal memory.

Yes, I know. I did it this way because I something broke under stress
when ugeth->confBd[txQ] instead. The ucc_geth_tx() and 
ucc_geth_start_xmit()
gets out of sync somehow.

> 
> We should really do some performance tests (using gbit links).
> I'll try to help you with the tests, but it might take some time.

Good, because I don't have GBE links :(

> 
> [...]
> > +      if (!in_be32((u32 __iomem *)(bd+4)))
> [...]
> > +      out_be32((u32 __iomem *)(bd+4), (int)NULL); /* Mark it free */
> 
> How about some inline function that will self-document the bd + 4
> stuff? Plus that way we'll get rid of the casts.

Good idea, will look at that.

> 
> Note that "bd+4" should be "bd + 4". And (int)NULL makes
> little sense, just 0 will work.

Sure, done.

^ permalink raw reply

* Re: [PATCH 1/2] ucc_geth: Move freeing of TX packets to NAPI context.
From: Joakim Tjernlund @ 2009-03-26 16:55 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: linuxppc-dev, leoli, netdev
In-Reply-To: <49CB8DFD.2050504@cosmosbay.com>

Eric Dumazet <dada1@cosmosbay.com> wrote on 26/03/2009 15:15:25:
>=20
> Joakim Tjernlund a =E9crit :
> > Also set NAPI weight to 64 as this is a common value.
> > This will make the system alot more responsive while
> > ping flooding the ucc=5Fgeth ethernet interaface.
> >=20
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> > ---
> >  drivers/net/ucc=5Fgeth.c |   32 ++++++++++++--------------------
> >  drivers/net/ucc=5Fgeth.h |    1 -
> >  2 files changed, 12 insertions(+), 21 deletions(-)
> >=20
> > diff --git a/drivers/net/ucc=5Fgeth.c b/drivers/net/ucc=5Fgeth.c
> > index 097aed8..7fc91aa 100644
> > --- a/drivers/net/ucc=5Fgeth.c
> > +++ b/drivers/net/ucc=5Fgeth.c
> > @@ -3214,7 +3214,7 @@ static int ucc=5Fgeth=5Ftx(struct net=5Fdevice *d=
ev,=20
u8 txQ)
> >        dev->stats.tx=5Fpackets++;
> >=20
> >        /* Free the sk buffer associated with this TxBD */
> > -      dev=5Fkfree=5Fskb=5Firq(ugeth->
> > +      dev=5Fkfree=5Fskb(ugeth->
> >                tx=5Fskbuff[txQ][ugeth->skb=5Fdirtytx[txQ]]);
> >        ugeth->tx=5Fskbuff[txQ][ugeth->skb=5Fdirtytx[txQ]] =3D NULL;
> >        ugeth->skb=5Fdirtytx[txQ] =3D
> > @@ -3248,9 +3248,16 @@ static int ucc=5Fgeth=5Fpoll(struct napi=5Fstruc=
t=20
*napi, int budget)
>=20
> >     for (i =3D 0; i < ug=5Finfo->numQueuesRx; i++)
> >        howmany +=3D ucc=5Fgeth=5Frx(ugeth, i, budget - howmany);
>=20
> Not related to your patch, but seeing above code, I can understand
> you might have a problem in flood situation : Only first queue(s) are
> depleted, and last one cannot be because howmany >=3D budget.
>=20
> This driver might have to remember last queue it handled at previous
> call ucc=5Fgeth=5Fpoll(), so that all rxqueues have same probability to b=
e=20
scanned.
>=20
> j =3D ug->lastslot;
> for (i =3D 0; ug=5Finfo->numQueuesRx; i++) {
>    if (++j >=3D ug=5Finfo->numQueuesRx)
>       j =3D 0;
>    howmany +=3D ucc=5Fgeth=5Frx(ugeth, j, budget - howmany);
>    if (howmany >=3D budget)
>       break;
> }
> ug->lastslot =3D j;

yes, or scan the RX queues in prio order. However ATM there is only
one queue so it won't be a problem until one extends the driver with
several queues.

Thanks,
        Jocke

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox