Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 16:01, Bryan Wu wrote:

> drivers/net/e100.c: ns->tx_window_errors += 
> le32_to_cpu(s->tx_late_collisions);
> drivers/net/e100.c: ns->tx_carrier_errors += 
> le32_to_cpu(s->tx_lost_crs);
> drivers/net/e100.c: ns->tx_fifo_errors += 
> le32_to_cpu(s->tx_underruns);
> drivers/net/e100.c: ns->tx_errors += 
> le32_to_cpu(s->tx_max_collisions) +
> drivers/net/e100.c: le32_to_cpu(s->tx_lost_crs);
> drivers/net/e100.c: ns->rx_length_errors += 
> le32_to_cpu(s->rx_short_frame_errors) +
> drivers/net/e100.c: ns->rx_crc_errors += 
> le32_to_cpu(s->rx_crc_errors);
> drivers/net/e100.c: ns->rx_frame_errors += 
> le32_to_cpu(s->rx_alignment_errors);
> drivers/net/e100.c: ns->rx_over_errors += 
> le32_to_cpu(s->rx_overrun_errors);
> drivers/net/e100.c: ns->rx_fifo_errors += 
> le32_to_cpu(s->rx_overrun_errors);
> drivers/net/e100.c: ns->rx_missed_errors += 
> le32_to_cpu(s->rx_resource_errors);
> drivers/net/e100.c: ns->rx_errors += 
> le32_to_cpu(s->rx_crc_errors) +
> drivers/net/e100.c: le32_to_cpu(s->rx_alignment_errors) +
> drivers/net/e100.c: le32_to_cpu(s->rx_short_frame_errors) 
> +
> drivers/net/e100.c: le32_to_cpu(s->rx_cdt_errors);
> drivers/net/e100.c: nic->tx_deferred += 
> le32_to_cpu(s->tx_deferred);
> drivers/net/e100.c: le32_to_cpu(s->tx_single_collisions);
> drivers/net/e100.c: 
> le32_to_cpu(s->tx_multiple_collisions);
> drivers/net/e100.c: nic->tx_fc_pause += 
> le32_to_cpu(s->fc_xmt_pause);
> drivers/net/e100.c: nic->rx_fc_pause += 
> le32_to_cpu(s->fc_rcv_pause);
> drivers/net/e100.c: 
> le32_to_cpu(s->fc_rcv_unsupported);
> drivers/net/e100.c: 
> le32_to_cpu(cb->u.tcb.tbd.buf_addr),
> drivers/net/e100.c: 
> le32_to_cpu(cb->u.tcb.tbd.buf_addr),
> ---
> 
> Normally, it is used to protect some rx/tx status flags or dma buf addr.
> 
> Any guide line for this leXX_to_cpu usage?

Easy.

leXX_to_cpu converts a little endian value to CPU-endianess.
So if your CPU is LE, this is a no-op.
The cpu_to_leXX func converts from CPU-endianess to LE.
Same goes for the bigendian variants of the funcs.

In drivers you often have values with specific endianess
(mostly LE, sometimes BE). For example when values are put
by the device to DMA memory. So if the device works in LE,
you must convert the value from LE to CPU endianess after
reading it from DMA.

I your case, however, the issue is different. You had a byte
array and did pointer casting tricks on it. Pointer casting
_is_ a common source for endianess issues. In general, if
you try to cast a pointer, think twice about it. It's likely
a bug. So you had a pointer to a byte array, which is u8*.
You casted that to u16* and u32*. That's broken, because
an u8 array is "little endian". LE means the least significant
part of the entity comes first in memory. Which is obviously
always true for a byte array. So you must use
leXX_to_cpu when doing this kind of pointer tricks.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Robert Hancock 

Bryan Wu wrote:

On Sun, 2007-07-15 at 14:17 +0200, Michael Buesch wrote:

On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)

 {
+   u32 addr_low = le32_to_cpu(*(u32 *) & mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(u16 *) & mac_addr[4]);
+
/* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)

@@ -866,10 +873,10 @@
int retval;
 
 	/* Grab the MAC address in the MAC */

-   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(u32 *) (&(dev->dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(u16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());

Try something like this:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)

 {
+   u32 addr_low = le32_to_cpu(*(__le32 *) & mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(__le16 *) & mac_addr[4]);
+
-   /* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)

@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */

-   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(__le32 *) (&(dev->dev_addr[0])) = 
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(__le16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());



Thanks a lot, Michael. 


I got a generic question about this endianess check. When should use it
in a driver or something else? I grep it in the driver/net/

---
drivers/net/e100.c: ns->tx_window_errors += 
le32_to_cpu(s->tx_late_collisions);
drivers/net/e100.c: ns->tx_carrier_errors += 
le32_to_cpu(s->tx_lost_crs);
drivers/net/e100.c: ns->tx_fifo_errors += 
le32_to_cpu(s->tx_underruns);
drivers/net/e100.c: ns->tx_errors += 
le32_to_cpu(s->tx_max_collisions) +
drivers/net/e100.c: le32_to_cpu(s->tx_lost_crs);
drivers/net/e100.c: ns->rx_length_errors += 
le32_to_cpu(s->rx_short_frame_errors) +
drivers/net/e100.c: ns->rx_crc_errors += 
le32_to_cpu(s->rx_crc_errors);
drivers/net/e100.c: ns->rx_frame_errors += 
le32_to_cpu(s->rx_alignment_errors);
drivers/net/e100.c: ns->rx_over_errors += 
le32_to_cpu(s->rx_overrun_errors);
drivers/net/e100.c: ns->rx_fifo_errors += 
le32_to_cpu(s->rx_overrun_errors);
drivers/net/e100.c: ns->rx_missed_errors += 
le32_to_cpu(s->rx_resource_errors);
drivers/net/e100.c: ns->rx_errors += le32_to_cpu(s->rx_crc_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_alignment_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_short_frame_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_cdt_errors);
drivers/net/e100.c: nic->tx_deferred += le32_to_cpu(s->tx_deferred);
drivers/net/e100.c: le32_to_cpu(s->tx_single_collisions);
drivers/net/e100.c: le32_to_cpu(s->tx_multiple_collisions);
drivers/net/e100.c: nic->tx_fc_pause += 
le32_to_cpu(s->fc_xmt_pause);
drivers/net/e100.c: nic->rx_fc_pause += 
le32_to_cpu(s->fc_rcv_pause);
drivers/net/e100.c: 
le32_to_cpu(s->fc_rcv_unsupported);
drivers/net/e100.c: 
le32_to_cpu(cb->u.tcb.tbd.buf_addr),
drivers/net/e100.c: 
le32_to_cpu(cb->u.tcb.tbd.buf_addr),
---

Normally, it is used to protect some rx/tx status flags or dma buf addr.

Any guide line for this leXX_to_cpu usage?


It has to be used when accessing any data structure stored in RAM that 
the device will access and where byte order is significant. cpu_to_le32 
when writing to the RAM, le32_to_cpu when reading it. (or le16, etc. if 
needed).


--
Robert Hancock  Saskatoon, SK, Canada
To email, remove "nospam" from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 14:17 +0200, Michael Buesch wrote:
> On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
> > @@ -483,9 +487,12 @@
> >  
> >  void setup_mac_addr(u8 * mac_addr)
> >  {
> > +   u32 addr_low = le32_to_cpu(*(u32 *) & mac_addr[0]);
> > +   u16 addr_hi = le16_to_cpu(*(u16 *) & mac_addr[4]);
> > +
> > /* this depends on a little-endian machine */
> > -   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
> > -   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
> > +   bfin_write_EMAC_ADDRLO(addr_low);
> > +   bfin_write_EMAC_ADDRHI(addr_hi);
> >  }
> >  
> >  static void adjust_tx_list(void)
> > @@ -866,10 +873,10 @@
> > int retval;
> >  
> > /* Grab the MAC address in the MAC */
> > -   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
> > -   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
> > +   *(u32 *) (&(dev->dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
> > +   *(u16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
> > bfin_read_EMAC_ADDRHI());
> 
> Try something like this:
> 
> @@ -483,9 +487,12 @@
>  
>  void setup_mac_addr(u8 * mac_addr)
>  {
> +   u32 addr_low = le32_to_cpu(*(__le32 *) & mac_addr[0]);
> +   u16 addr_hi = le16_to_cpu(*(__le16 *) & mac_addr[4]);
> +
> -   /* this depends on a little-endian machine */
> -   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
> -   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
> +   bfin_write_EMAC_ADDRLO(addr_low);
> +   bfin_write_EMAC_ADDRHI(addr_hi);
>  }
>  
>  static void adjust_tx_list(void)
> @@ -866,10 +873,10 @@
> int retval;
>  
> /* Grab the MAC address in the MAC */
> -   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
> -   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
> +   *(__le32 *) (&(dev->dev_addr[0])) = 
> cpu_to_le32(bfin_read_EMAC_ADDRLO());
> +   *(__le16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
> bfin_read_EMAC_ADDRHI());
> 

Thanks a lot, Michael. 

I got a generic question about this endianess check. When should use it
in a driver or something else? I grep it in the driver/net/

---
drivers/net/e100.c: ns->tx_window_errors += 
le32_to_cpu(s->tx_late_collisions);
drivers/net/e100.c: ns->tx_carrier_errors += 
le32_to_cpu(s->tx_lost_crs);
drivers/net/e100.c: ns->tx_fifo_errors += 
le32_to_cpu(s->tx_underruns);
drivers/net/e100.c: ns->tx_errors += 
le32_to_cpu(s->tx_max_collisions) +
drivers/net/e100.c: le32_to_cpu(s->tx_lost_crs);
drivers/net/e100.c: ns->rx_length_errors += 
le32_to_cpu(s->rx_short_frame_errors) +
drivers/net/e100.c: ns->rx_crc_errors += 
le32_to_cpu(s->rx_crc_errors);
drivers/net/e100.c: ns->rx_frame_errors += 
le32_to_cpu(s->rx_alignment_errors);
drivers/net/e100.c: ns->rx_over_errors += 
le32_to_cpu(s->rx_overrun_errors);
drivers/net/e100.c: ns->rx_fifo_errors += 
le32_to_cpu(s->rx_overrun_errors);
drivers/net/e100.c: ns->rx_missed_errors += 
le32_to_cpu(s->rx_resource_errors);
drivers/net/e100.c: ns->rx_errors += le32_to_cpu(s->rx_crc_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_alignment_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_short_frame_errors) +
drivers/net/e100.c: le32_to_cpu(s->rx_cdt_errors);
drivers/net/e100.c: nic->tx_deferred += le32_to_cpu(s->tx_deferred);
drivers/net/e100.c: le32_to_cpu(s->tx_single_collisions);
drivers/net/e100.c: le32_to_cpu(s->tx_multiple_collisions);
drivers/net/e100.c: nic->tx_fc_pause += 
le32_to_cpu(s->fc_xmt_pause);
drivers/net/e100.c: nic->rx_fc_pause += 
le32_to_cpu(s->fc_rcv_pause);
drivers/net/e100.c: 
le32_to_cpu(s->fc_rcv_unsupported);
drivers/net/e100.c: 
le32_to_cpu(cb->u.tcb.tbd.buf_addr),
drivers/net/e100.c: 
le32_to_cpu(cb->u.tcb.tbd.buf_addr),
---

Normally, it is used to protect some rx/tx status flags or dma buf addr.

Any guide line for this leXX_to_cpu usage?

Thanks again

- Bryan Wu
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
> @@ -483,9 +487,12 @@
>  
>  void setup_mac_addr(u8 * mac_addr)
>  {
> + u32 addr_low = le32_to_cpu(*(u32 *) & mac_addr[0]);
> + u16 addr_hi = le16_to_cpu(*(u16 *) & mac_addr[4]);
> +
>   /* this depends on a little-endian machine */
> - bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
> - bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
> + bfin_write_EMAC_ADDRLO(addr_low);
> + bfin_write_EMAC_ADDRHI(addr_hi);
>  }
>  
>  static void adjust_tx_list(void)
> @@ -866,10 +873,10 @@
>   int retval;
>  
>   /* Grab the MAC address in the MAC */
> - *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
> - *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
> + *(u32 *) (&(dev->dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
> + *(u16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
> bfin_read_EMAC_ADDRHI());

Try something like this:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)
 {
+   u32 addr_low = le32_to_cpu(*(__le32 *) & mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(__le16 *) & mac_addr[4]);
+
-   /* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */
-   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(__le32 *) (&(dev->dev_addr[0])) = 
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(__le16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());

-- 
Greetings Michael.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 11:53 +0100, Christoph Hellwig wrote:
> On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote:
> > On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
> > > +#if defined(CONFIG_BFIN_MAC_USE_L1)
> > > +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
> > > +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
> > > +#else
> > > +# define bfin_mac_alloc(dma_handle, size) \
> > > +  dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
> > 
> > What is GFP_NORMAL? It's not defined in latest linus' tree.
> > I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
> > if you can't.
> 
> Actually this whole thing looks fishy.  There should be a struct device
> for the dma allocation, through a platform_device.  And the
> CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma
> provider so this can be handled through the dma api.

We do have L1 memory DMA allocator and manager, it is depends on
Blackfin arch.
And this driver is Blackfin on-chip ethernet MAC controller. 

So it is OK, any idea?

Thanks
- Bryan Wu
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 12:36 +0200, Michael Buesch wrote:
> On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
> > +#if defined(CONFIG_BFIN_MAC_USE_L1)
> > +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
> > +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
> > +#else
> > +# define bfin_mac_alloc(dma_handle, size) \
> > +dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
> 
> What is GFP_NORMAL? It's not defined in latest linus' tree.
> I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
> if you can't.
> 

Yes, it should be GFP_KERNEL, i missed up with ZONE_NORMAL and ZONE_DMA.

> The rest looks OK. Except the endianess issues. It might be no
> issue on the hardware this runs on, but in favour of "clean code"
> you might use leXX_to_cpu and friends anyway. :)
> This kind of bugs is done so often, even in places where it _does_
> matter. So, at least for the human reader, the leXX_to_cpu stuff
> says that you really understood what you were doing when writing
> the code. The current code says (to me), that it works by
> accident, somehow, although it seems you knew what you were doing. :)
> 

As you know, this driver is original developed by Luke Yang. Now I am
maintaining it.
I just don't understand in this driver where should use leXX_to_cpu()
and where should
use cpu_to_leXX(). please give me some comments about the following
change:

---
@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)
 {
+   u32 addr_low = le32_to_cpu(*(u32 *) & mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(u16 *) & mac_addr[4]);
+
/* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *) & mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *) & mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */
-   *(u32 *) (&(dev->dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) (&(dev->dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(u32 *) (&(dev->dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(u16 *) (&(dev->dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());
---

Thanks
- Bryan Wu
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Christoph Hellwig 
On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote:
> On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
> > +#if defined(CONFIG_BFIN_MAC_USE_L1)
> > +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
> > +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
> > +#else
> > +# define bfin_mac_alloc(dma_handle, size) \
> > +dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
> 
> What is GFP_NORMAL? It's not defined in latest linus' tree.
> I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
> if you can't.

Actually this whole thing looks fishy.  There should be a struct device
for the dma allocation, through a platform_device.  And the
CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma
provider so this can be handled through the dma api.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
> +#if defined(CONFIG_BFIN_MAC_USE_L1)
> +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
> +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
> +#else
> +# define bfin_mac_alloc(dma_handle, size) \
> +  dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)

What is GFP_NORMAL? It's not defined in latest linus' tree.
I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
if you can't.

The rest looks OK. Except the endianess issues. It might be no
issue on the hardware this runs on, but in favour of "clean code"
you might use leXX_to_cpu and friends anyway. :)
This kind of bugs is done so often, even in places where it _does_
matter. So, at least for the human reader, the leXX_to_cpu stuff
says that you really understood what you were doing when writing
the code. The current code says (to me), that it works by
accident, somehow, although it seems you knew what you were doing. :)

-- 
Greetings Michael.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
This patch implements the driver necessary use the Analog Devices
Blackfin processor's on-chip ethernet MAC controller.

 - add timeout control
 - kill dma_config_reg bitfields
 - some trivial cleanup 

Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
Cc: Michael Buesch <[EMAIL PROTECTED]>
Cc: Mike Frysinger <[EMAIL PROTECTED]> 
Cc: Jeff Garzik <[EMAIL PROTECTED]>
Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
---
 MAINTAINERS|7 +
 drivers/net/Kconfig|   44 ++
 drivers/net/Makefile   |1 +
 drivers/net/bfin_mac.c | 1016 
 drivers/net/bfin_mac.h |  132 +++
 5 files changed, 1200 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/bfin_mac.c
 create mode 100644 drivers/net/bfin_mac.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 845fbf4..21a2265 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -754,6 +754,13 @@ L: [EMAIL PROTECTED] (subscribers-only)
 W: http://blackfin.uclinux.org
 S: Supported
 
+BLACKFIN EMAC DRIVER
+P: Bryan Wu
+M: [EMAIL PROTECTED]
+L: [EMAIL PROTECTED] (subscribers-only)
+W: http://blackfin.uclinux.org
+S: Supported
+
 BLACKFIN RTC DRIVER
 P: Mike Frysinger
 M: [EMAIL PROTECTED]
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index ba314ad..9356a6e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -838,6 +838,50 @@ config ULTRA32
  . The module
  will be called smc-ultra32.
 
+config BFIN_MAC
+   tristate "Blackfin 536/537 on-chip mac support"
+   depends on NET_ETHERNET && (BF537 || BF536) && (!BF537_PORT_H)
+   select CRC32
+   select BFIN_MAC_USE_L1 if DMA_UNCACHED_NONE
+   help
+ This is the driver for blackfin on-chip mac device. Say Y if you want 
it
+ compiled into the kernel. This driver is also available as a module
+ ( = code which can be inserted in and removed from the running kernel
+ whenever you want). The module will be called bfin_mac.
+
+config BFIN_MAC_USE_L1
+   bool "Use L1 memory for rx/tx packets"
+   depends on BFIN_MAC && BF537
+   default y
+   help
+ To get maximum network performace, you should use L1 memory as rx/tx 
buffers.
+ Say N here if you want to reserve L1 memory for other uses.
+
+config BFIN_TX_DESC_NUM
+   int "Number of transmit buffer packets"
+   depends on BFIN_MAC
+   range 6 10 if BFIN_MAC_USE_L1
+   range 10 100
+   default "10"
+   help
+ Set the number of buffer packets used in driver.
+
+config BFIN_RX_DESC_NUM
+   int "Number of receive buffer packets"
+   depends on BFIN_MAC
+   range 20 100 if BFIN_MAC_USE_L1
+   range 20 800
+   default "20"
+   help
+ Set the number of buffer packets used in driver.
+
+config BFIN_MAC_RMII
+   bool "RMII PHY Interface (EXPERIMENTAL)"
+   depends on BFIN_MAC && EXPERIMENTAL
+   default n
+   help
+ Use Reduced PHY MII Interface
+
 config SMC9194
tristate "SMC 9194 support"
depends on NET_VENDOR_SMC && (ISA || MAC && BROKEN)
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index a2241e6..c23a1b3 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -198,6 +198,7 @@ obj-$(CONFIG_S2IO) += s2io.o
 obj-$(CONFIG_MYRI10GE) += myri10ge/
 obj-$(CONFIG_SMC91X) += smc91x.o
 obj-$(CONFIG_SMC911X) += smc911x.o
+obj-$(CONFIG_BFIN_MAC) += bfin_mac.o
 obj-$(CONFIG_DM9000) += dm9000.o
 obj-$(CONFIG_FEC_8XX) += fec_8xx/
 obj-$(CONFIG_PASEMI_MAC) += pasemi_mac.o
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
new file mode 100644
index 000..6eb2794
--- /dev/null
+++ b/drivers/net/bfin_mac.c
@@ -0,0 +1,1016 @@
+/*
+ * File:   drivers/net/bfin_mac.c
+ * Based on:
+ * Maintainer:
+ * Bryan Wu <[EMAIL PROTECTED]>
+ *
+ * Original author:
+ * Luke Yang <[EMAIL PROTECTED]>
+ *
+ * Created:
+ * Description:
+ *
+ * Modified:
+ * Copyright 2004-2006 Analog Devices Inc.
+ *
+ * Bugs:   Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software ;  you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation ;  either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY ;  without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program ;  see the file COPYING.
+ * If not, write to the Free Software Foundation,
+ * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include

[PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
This patch implements the driver necessary use the Analog Devices
Blackfin processor's on-chip ethernet MAC controller.

 - add timeout control
 - kill dma_config_reg bitfields
 - some trivial cleanup 

Signed-off-by: Bryan Wu [EMAIL PROTECTED]
Cc: Michael Buesch [EMAIL PROTECTED]
Cc: Mike Frysinger [EMAIL PROTECTED] 
Cc: Jeff Garzik [EMAIL PROTECTED]
Signed-off-by: Andrew Morton [EMAIL PROTECTED]
---
 MAINTAINERS|7 +
 drivers/net/Kconfig|   44 ++
 drivers/net/Makefile   |1 +
 drivers/net/bfin_mac.c | 1016 
 drivers/net/bfin_mac.h |  132 +++
 5 files changed, 1200 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/bfin_mac.c
 create mode 100644 drivers/net/bfin_mac.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 845fbf4..21a2265 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -754,6 +754,13 @@ L: [EMAIL PROTECTED] (subscribers-only)
 W: http://blackfin.uclinux.org
 S: Supported
 
+BLACKFIN EMAC DRIVER
+P: Bryan Wu
+M: [EMAIL PROTECTED]
+L: [EMAIL PROTECTED] (subscribers-only)
+W: http://blackfin.uclinux.org
+S: Supported
+
 BLACKFIN RTC DRIVER
 P: Mike Frysinger
 M: [EMAIL PROTECTED]
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index ba314ad..9356a6e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -838,6 +838,50 @@ config ULTRA32
  file:Documentation/networking/net-modules.txt. The module
  will be called smc-ultra32.
 
+config BFIN_MAC
+   tristate Blackfin 536/537 on-chip mac support
+   depends on NET_ETHERNET  (BF537 || BF536)  (!BF537_PORT_H)
+   select CRC32
+   select BFIN_MAC_USE_L1 if DMA_UNCACHED_NONE
+   help
+ This is the driver for blackfin on-chip mac device. Say Y if you want 
it
+ compiled into the kernel. This driver is also available as a module
+ ( = code which can be inserted in and removed from the running kernel
+ whenever you want). The module will be called bfin_mac.
+
+config BFIN_MAC_USE_L1
+   bool Use L1 memory for rx/tx packets
+   depends on BFIN_MAC  BF537
+   default y
+   help
+ To get maximum network performace, you should use L1 memory as rx/tx 
buffers.
+ Say N here if you want to reserve L1 memory for other uses.
+
+config BFIN_TX_DESC_NUM
+   int Number of transmit buffer packets
+   depends on BFIN_MAC
+   range 6 10 if BFIN_MAC_USE_L1
+   range 10 100
+   default 10
+   help
+ Set the number of buffer packets used in driver.
+
+config BFIN_RX_DESC_NUM
+   int Number of receive buffer packets
+   depends on BFIN_MAC
+   range 20 100 if BFIN_MAC_USE_L1
+   range 20 800
+   default 20
+   help
+ Set the number of buffer packets used in driver.
+
+config BFIN_MAC_RMII
+   bool RMII PHY Interface (EXPERIMENTAL)
+   depends on BFIN_MAC  EXPERIMENTAL
+   default n
+   help
+ Use Reduced PHY MII Interface
+
 config SMC9194
tristate SMC 9194 support
depends on NET_VENDOR_SMC  (ISA || MAC  BROKEN)
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index a2241e6..c23a1b3 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -198,6 +198,7 @@ obj-$(CONFIG_S2IO) += s2io.o
 obj-$(CONFIG_MYRI10GE) += myri10ge/
 obj-$(CONFIG_SMC91X) += smc91x.o
 obj-$(CONFIG_SMC911X) += smc911x.o
+obj-$(CONFIG_BFIN_MAC) += bfin_mac.o
 obj-$(CONFIG_DM9000) += dm9000.o
 obj-$(CONFIG_FEC_8XX) += fec_8xx/
 obj-$(CONFIG_PASEMI_MAC) += pasemi_mac.o
diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
new file mode 100644
index 000..6eb2794
--- /dev/null
+++ b/drivers/net/bfin_mac.c
@@ -0,0 +1,1016 @@
+/*
+ * File:   drivers/net/bfin_mac.c
+ * Based on:
+ * Maintainer:
+ * Bryan Wu [EMAIL PROTECTED]
+ *
+ * Original author:
+ * Luke Yang [EMAIL PROTECTED]
+ *
+ * Created:
+ * Description:
+ *
+ * Modified:
+ * Copyright 2004-2006 Analog Devices Inc.
+ *
+ * Bugs:   Enter bugs at http://blackfin.uclinux.org/
+ *
+ * This program is free software ;  you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation ;  either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY ;  without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program ;  see the file COPYING.
+ * If not, write to the Free Software Foundation,
+ * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include linux/kernel.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/delay.h

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
 +#if defined(CONFIG_BFIN_MAC_USE_L1)
 +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
 +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
 +#else
 +# define bfin_mac_alloc(dma_handle, size) \
 +  dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)

What is GFP_NORMAL? It's not defined in latest linus' tree.
I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
if you can't.

The rest looks OK. Except the endianess issues. It might be no
issue on the hardware this runs on, but in favour of clean code
you might use leXX_to_cpu and friends anyway. :)
This kind of bugs is done so often, even in places where it _does_
matter. So, at least for the human reader, the leXX_to_cpu stuff
says that you really understood what you were doing when writing
the code. The current code says (to me), that it works by
accident, somehow, although it seems you knew what you were doing. :)

-- 
Greetings Michael.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Christoph Hellwig 
On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote:
 On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
  +#if defined(CONFIG_BFIN_MAC_USE_L1)
  +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
  +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
  +#else
  +# define bfin_mac_alloc(dma_handle, size) \
  +dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
 
 What is GFP_NORMAL? It's not defined in latest linus' tree.
 I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
 if you can't.

Actually this whole thing looks fishy.  There should be a struct device
for the dma allocation, through a platform_device.  And the
CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma
provider so this can be handled through the dma api.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 12:36 +0200, Michael Buesch wrote:
 On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
  +#if defined(CONFIG_BFIN_MAC_USE_L1)
  +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
  +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
  +#else
  +# define bfin_mac_alloc(dma_handle, size) \
  +dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
 
 What is GFP_NORMAL? It's not defined in latest linus' tree.
 I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
 if you can't.
 

Yes, it should be GFP_KERNEL, i missed up with ZONE_NORMAL and ZONE_DMA.

 The rest looks OK. Except the endianess issues. It might be no
 issue on the hardware this runs on, but in favour of clean code
 you might use leXX_to_cpu and friends anyway. :)
 This kind of bugs is done so often, even in places where it _does_
 matter. So, at least for the human reader, the leXX_to_cpu stuff
 says that you really understood what you were doing when writing
 the code. The current code says (to me), that it works by
 accident, somehow, although it seems you knew what you were doing. :)
 

As you know, this driver is original developed by Luke Yang. Now I am
maintaining it.
I just don't understand in this driver where should use leXX_to_cpu()
and where should
use cpu_to_leXX(). please give me some comments about the following
change:

---
@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)
 {
+   u32 addr_low = le32_to_cpu(*(u32 *)  mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(u16 *)  mac_addr[4]);
+
/* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */
-   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());
---

Thanks
- Bryan Wu
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 11:53 +0100, Christoph Hellwig wrote:
 On Sun, Jul 15, 2007 at 12:36:51PM +0200, Michael Buesch wrote:
  On Sunday 15 July 2007 11:27:09 Bryan Wu wrote:
   +#if defined(CONFIG_BFIN_MAC_USE_L1)
   +# define bfin_mac_alloc(dma_handle, size)  l1_data_sram_zalloc(size)
   +# define bfin_mac_free(dma_handle, ptr)l1_data_sram_free(ptr)
   +#else
   +# define bfin_mac_alloc(dma_handle, size) \
   +  dma_alloc_coherent(NULL, size, dma_handle, GFP_NORMAL)
  
  What is GFP_NORMAL? It's not defined in latest linus' tree.
  I think you should use GFP_KERNEL, if you can sleep, or GFP_ATOMIC,
  if you can't.
 
 Actually this whole thing looks fishy.  There should be a struct device
 for the dma allocation, through a platform_device.  And the
 CONFIG_BFIN_MAC_USE_L1 should go away, the l1 sram should have a dma
 provider so this can be handled through the dma api.

We do have L1 memory DMA allocator and manager, it is depends on
Blackfin arch.
And this driver is Blackfin on-chip ethernet MAC controller. 

So it is OK, any idea?

Thanks
- Bryan Wu
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
 @@ -483,9 +487,12 @@
  
  void setup_mac_addr(u8 * mac_addr)
  {
 + u32 addr_low = le32_to_cpu(*(u32 *)  mac_addr[0]);
 + u16 addr_hi = le16_to_cpu(*(u16 *)  mac_addr[4]);
 +
   /* this depends on a little-endian machine */
 - bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
 - bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
 + bfin_write_EMAC_ADDRLO(addr_low);
 + bfin_write_EMAC_ADDRHI(addr_hi);
  }
  
  static void adjust_tx_list(void)
 @@ -866,10 +873,10 @@
   int retval;
  
   /* Grab the MAC address in the MAC */
 - *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
 - *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
 + *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
 + *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
 bfin_read_EMAC_ADDRHI());

Try something like this:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)
 {
+   u32 addr_low = le32_to_cpu(*(__le32 *)  mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(__le16 *)  mac_addr[4]);
+
-   /* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)
@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */
-   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(__le32 *) ((dev-dev_addr[0])) = 
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(__le16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());

-- 
Greetings Michael.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Bryan Wu 
On Sun, 2007-07-15 at 14:17 +0200, Michael Buesch wrote:
 On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:
  @@ -483,9 +487,12 @@
   
   void setup_mac_addr(u8 * mac_addr)
   {
  +   u32 addr_low = le32_to_cpu(*(u32 *)  mac_addr[0]);
  +   u16 addr_hi = le16_to_cpu(*(u16 *)  mac_addr[4]);
  +
  /* this depends on a little-endian machine */
  -   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
  -   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
  +   bfin_write_EMAC_ADDRLO(addr_low);
  +   bfin_write_EMAC_ADDRHI(addr_hi);
   }
   
   static void adjust_tx_list(void)
  @@ -866,10 +873,10 @@
  int retval;
   
  /* Grab the MAC address in the MAC */
  -   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
  -   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
  +   *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
  +   *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
  bfin_read_EMAC_ADDRHI());
 
 Try something like this:
 
 @@ -483,9 +487,12 @@
  
  void setup_mac_addr(u8 * mac_addr)
  {
 +   u32 addr_low = le32_to_cpu(*(__le32 *)  mac_addr[0]);
 +   u16 addr_hi = le16_to_cpu(*(__le16 *)  mac_addr[4]);
 +
 -   /* this depends on a little-endian machine */
 -   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
 -   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
 +   bfin_write_EMAC_ADDRLO(addr_low);
 +   bfin_write_EMAC_ADDRHI(addr_hi);
  }
  
  static void adjust_tx_list(void)
 @@ -866,10 +873,10 @@
 int retval;
  
 /* Grab the MAC address in the MAC */
 -   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
 -   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
 +   *(__le32 *) ((dev-dev_addr[0])) = 
 cpu_to_le32(bfin_read_EMAC_ADDRLO());
 +   *(__le16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
 bfin_read_EMAC_ADDRHI());
 

Thanks a lot, Michael. 

I got a generic question about this endianess check. When should use it
in a driver or something else? I grep it in the driver/net/

---
drivers/net/e100.c: ns-tx_window_errors += 
le32_to_cpu(s-tx_late_collisions);
drivers/net/e100.c: ns-tx_carrier_errors += 
le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-tx_fifo_errors += 
le32_to_cpu(s-tx_underruns);
drivers/net/e100.c: ns-tx_errors += 
le32_to_cpu(s-tx_max_collisions) +
drivers/net/e100.c: le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-rx_length_errors += 
le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: ns-rx_crc_errors += 
le32_to_cpu(s-rx_crc_errors);
drivers/net/e100.c: ns-rx_frame_errors += 
le32_to_cpu(s-rx_alignment_errors);
drivers/net/e100.c: ns-rx_over_errors += 
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_fifo_errors += 
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_missed_errors += 
le32_to_cpu(s-rx_resource_errors);
drivers/net/e100.c: ns-rx_errors += le32_to_cpu(s-rx_crc_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_alignment_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_cdt_errors);
drivers/net/e100.c: nic-tx_deferred += le32_to_cpu(s-tx_deferred);
drivers/net/e100.c: le32_to_cpu(s-tx_single_collisions);
drivers/net/e100.c: le32_to_cpu(s-tx_multiple_collisions);
drivers/net/e100.c: nic-tx_fc_pause += 
le32_to_cpu(s-fc_xmt_pause);
drivers/net/e100.c: nic-rx_fc_pause += 
le32_to_cpu(s-fc_rcv_pause);
drivers/net/e100.c: 
le32_to_cpu(s-fc_rcv_unsupported);
drivers/net/e100.c: 
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
drivers/net/e100.c: 
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
---

Normally, it is used to protect some rx/tx status flags or dma buf addr.

Any guide line for this leXX_to_cpu usage?

Thanks again

- Bryan Wu
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Robert Hancock 

Bryan Wu wrote:

On Sun, 2007-07-15 at 14:17 +0200, Michael Buesch wrote:

On Sunday 15 July 2007 14:07:44 Bryan Wu wrote:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)

 {
+   u32 addr_low = le32_to_cpu(*(u32 *)  mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(u16 *)  mac_addr[4]);
+
/* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)

@@ -866,10 +873,10 @@
int retval;
 
 	/* Grab the MAC address in the MAC */

-   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(u32 *) ((dev-dev_addr[0])) = cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(u16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());

Try something like this:

@@ -483,9 +487,12 @@
 
 void setup_mac_addr(u8 * mac_addr)

 {
+   u32 addr_low = le32_to_cpu(*(__le32 *)  mac_addr[0]);
+   u16 addr_hi = le16_to_cpu(*(__le16 *)  mac_addr[4]);
+
-   /* this depends on a little-endian machine */
-   bfin_write_EMAC_ADDRLO(*(u32 *)  mac_addr[0]);
-   bfin_write_EMAC_ADDRHI(*(u16 *)  mac_addr[4]);
+   bfin_write_EMAC_ADDRLO(addr_low);
+   bfin_write_EMAC_ADDRHI(addr_hi);
 }
 
 static void adjust_tx_list(void)

@@ -866,10 +873,10 @@
int retval;
 
/* Grab the MAC address in the MAC */

-   *(u32 *) ((dev-dev_addr[0])) = bfin_read_EMAC_ADDRLO();
-   *(u16 *) ((dev-dev_addr[4])) = (u16) bfin_read_EMAC_ADDRHI();
+   *(__le32 *) ((dev-dev_addr[0])) = 
cpu_to_le32(bfin_read_EMAC_ADDRLO());
+   *(__le16 *) ((dev-dev_addr[4])) = cpu_to_le16((u16) 
bfin_read_EMAC_ADDRHI());



Thanks a lot, Michael. 


I got a generic question about this endianess check. When should use it
in a driver or something else? I grep it in the driver/net/

---
drivers/net/e100.c: ns-tx_window_errors += 
le32_to_cpu(s-tx_late_collisions);
drivers/net/e100.c: ns-tx_carrier_errors += 
le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-tx_fifo_errors += 
le32_to_cpu(s-tx_underruns);
drivers/net/e100.c: ns-tx_errors += 
le32_to_cpu(s-tx_max_collisions) +
drivers/net/e100.c: le32_to_cpu(s-tx_lost_crs);
drivers/net/e100.c: ns-rx_length_errors += 
le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: ns-rx_crc_errors += 
le32_to_cpu(s-rx_crc_errors);
drivers/net/e100.c: ns-rx_frame_errors += 
le32_to_cpu(s-rx_alignment_errors);
drivers/net/e100.c: ns-rx_over_errors += 
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_fifo_errors += 
le32_to_cpu(s-rx_overrun_errors);
drivers/net/e100.c: ns-rx_missed_errors += 
le32_to_cpu(s-rx_resource_errors);
drivers/net/e100.c: ns-rx_errors += le32_to_cpu(s-rx_crc_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_alignment_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_short_frame_errors) +
drivers/net/e100.c: le32_to_cpu(s-rx_cdt_errors);
drivers/net/e100.c: nic-tx_deferred += le32_to_cpu(s-tx_deferred);
drivers/net/e100.c: le32_to_cpu(s-tx_single_collisions);
drivers/net/e100.c: le32_to_cpu(s-tx_multiple_collisions);
drivers/net/e100.c: nic-tx_fc_pause += 
le32_to_cpu(s-fc_xmt_pause);
drivers/net/e100.c: nic-rx_fc_pause += 
le32_to_cpu(s-fc_rcv_pause);
drivers/net/e100.c: 
le32_to_cpu(s-fc_rcv_unsupported);
drivers/net/e100.c: 
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
drivers/net/e100.c: 
le32_to_cpu(cb-u.tcb.tbd.buf_addr),
---

Normally, it is used to protect some rx/tx status flags or dma buf addr.

Any guide line for this leXX_to_cpu usage?


It has to be used when accessing any data structure stored in RAM that 
the device will access and where byte order is significant. cpu_to_le32 
when writing to the RAM, le32_to_cpu when reading it. (or le16, etc. if 
needed).


--
Robert Hancock  Saskatoon, SK, Canada
To email, remove nospam from [EMAIL PROTECTED]
Home Page: http://www.roberthancock.com/

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH try#2] Blackfin ethernet driver: on chip ethernet MAC controller driver

2007-07-15 Thread Michael Buesch 
On Sunday 15 July 2007 16:01, Bryan Wu wrote:

 drivers/net/e100.c: ns-tx_window_errors += 
 le32_to_cpu(s-tx_late_collisions);
 drivers/net/e100.c: ns-tx_carrier_errors += 
 le32_to_cpu(s-tx_lost_crs);
 drivers/net/e100.c: ns-tx_fifo_errors += 
 le32_to_cpu(s-tx_underruns);
 drivers/net/e100.c: ns-tx_errors += 
 le32_to_cpu(s-tx_max_collisions) +
 drivers/net/e100.c: le32_to_cpu(s-tx_lost_crs);
 drivers/net/e100.c: ns-rx_length_errors += 
 le32_to_cpu(s-rx_short_frame_errors) +
 drivers/net/e100.c: ns-rx_crc_errors += 
 le32_to_cpu(s-rx_crc_errors);
 drivers/net/e100.c: ns-rx_frame_errors += 
 le32_to_cpu(s-rx_alignment_errors);
 drivers/net/e100.c: ns-rx_over_errors += 
 le32_to_cpu(s-rx_overrun_errors);
 drivers/net/e100.c: ns-rx_fifo_errors += 
 le32_to_cpu(s-rx_overrun_errors);
 drivers/net/e100.c: ns-rx_missed_errors += 
 le32_to_cpu(s-rx_resource_errors);
 drivers/net/e100.c: ns-rx_errors += 
 le32_to_cpu(s-rx_crc_errors) +
 drivers/net/e100.c: le32_to_cpu(s-rx_alignment_errors) +
 drivers/net/e100.c: le32_to_cpu(s-rx_short_frame_errors) 
 +
 drivers/net/e100.c: le32_to_cpu(s-rx_cdt_errors);
 drivers/net/e100.c: nic-tx_deferred += 
 le32_to_cpu(s-tx_deferred);
 drivers/net/e100.c: le32_to_cpu(s-tx_single_collisions);
 drivers/net/e100.c: 
 le32_to_cpu(s-tx_multiple_collisions);
 drivers/net/e100.c: nic-tx_fc_pause += 
 le32_to_cpu(s-fc_xmt_pause);
 drivers/net/e100.c: nic-rx_fc_pause += 
 le32_to_cpu(s-fc_rcv_pause);
 drivers/net/e100.c: 
 le32_to_cpu(s-fc_rcv_unsupported);
 drivers/net/e100.c: 
 le32_to_cpu(cb-u.tcb.tbd.buf_addr),
 drivers/net/e100.c: 
 le32_to_cpu(cb-u.tcb.tbd.buf_addr),
 ---
 
 Normally, it is used to protect some rx/tx status flags or dma buf addr.
 
 Any guide line for this leXX_to_cpu usage?

Easy.

leXX_to_cpu converts a little endian value to CPU-endianess.
So if your CPU is LE, this is a no-op.
The cpu_to_leXX func converts from CPU-endianess to LE.
Same goes for the bigendian variants of the funcs.

In drivers you often have values with specific endianess
(mostly LE, sometimes BE). For example when values are put
by the device to DMA memory. So if the device works in LE,
you must convert the value from LE to CPU endianess after
reading it from DMA.

I your case, however, the issue is different. You had a byte
array and did pointer casting tricks on it. Pointer casting
_is_ a common source for endianess issues. In general, if
you try to cast a pointer, think twice about it. It's likely
a bug. So you had a pointer to a byte array, which is u8*.
You casted that to u16* and u32*. That's broken, because
an u8 array is little endian. LE means the least significant
part of the entity comes first in memory. Which is obviously
always true for a byte array. So you must use
leXX_to_cpu when doing this kind of pointer tricks.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/