[U-Boot] Remove board specific code from ENC28J60 network driver?

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Dirk Behme]

For TI OMAP3 Beagle based Zippy expansion board from TinCanTools [1] 
I'm currently looking into reusing spi based ENC28J60 network driver

drivers/net/enc28j60.c

It seems to me that it uses LPC2292 specific macros

IO1CLR, IO1SET and IO1DIR

These macros are defined in

asm-arm/arch-lpc2292/lpc2292_registers.h

 From enc28j60.c:

...
#define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
#define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
...

...
/* configure GPIO */
(*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
(*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;

/* CS and RESET active low */
PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
PUT32 (IO1SET, ENC_RESET);
...

Anybody with an idea how to move this code to some (LPC2292?) board 
specific files to make enc28j60.c more generic to be able to reuse it 
on other boards?

Best regards

Dirk

[1] 
http://www.tincantools.com/product.php?productid=16147&cat=0&page=1&featured

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Ben Warren]

Dirk,

On Sun, Dec 20, 2009 at 11:30 AM, Dirk Behme <dirk.behme@googlemail.com>wrote:

>
> For TI OMAP3 Beagle based Zippy expansion board from TinCanTools [1]
> I'm currently looking into reusing spi based ENC28J60 network driver
>
> drivers/net/enc28j60.c
>
> It seems to me that it uses LPC2292 specific macros
>
> IO1CLR, IO1SET and IO1DIR
>
> These macros are defined in
>
> asm-arm/arch-lpc2292/lpc2292_registers.h
>
>  From enc28j60.c:
>
> ...
> #define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
> #define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
> ...
>
> ...
> /* configure GPIO */
> (*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
> (*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;
>
> /* CS and RESET active low */
> PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
> PUT32 (IO1SET, ENC_RESET);
> ...
>
> Anybody with an idea how to move this code to some (LPC2292?) board
> specific files to make enc28j60.c more generic to be able to reuse it
> on other boards?
>
> From my brief glimpse I would think these should be changed to use the SPI
framework.  Then board code is only concerned with configuring SPI properly.


> Best regards
>
> Dirk
>
> regards,
Ben

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Mike Frysinger]

On Sunday 20 December 2009 14:30:35 Dirk Behme wrote:
> For TI OMAP3 Beagle based Zippy expansion board from TinCanTools [1]
> I'm currently looking into reusing spi based ENC28J60 network driver
> 
> drivers/net/enc28j60.c
> 
> It seems to me that it uses LPC2292 specific macros
> 
> IO1CLR, IO1SET and IO1DIR
> 
> These macros are defined in
> 
> asm-arm/arch-lpc2292/lpc2292_registers.h

this is why we didnt bother trying to get this part working on Blackfin boards 
under u-boot.  it works fine for us under Linux, but the u-boot driver is a 
joke.

>  From enc28j60.c:
> 
> ...
> #define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
> #define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
> ...
> 
> ...
> /* configure GPIO */
> (*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
> (*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;
> 
> /* CS and RESET active low */
> PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
> PUT32 (IO1SET, ENC_RESET);
> ...
> 
> Anybody with an idea how to move this code to some (LPC2292?) board
> specific files to make enc28j60.c more generic to be able to reuse it
> on other boards?

unless the maintainers of the LPC2292 board are willing to help, i'd say just 
avoid the issue:
 - rename/move this driver to indicate it is specific to LPC2292
 - create a new driver based on the old one using the common SPI framework

once it starts using the common SPI framework, i should be able to easily help 
with testing on Blackfin boards.  we have a little addon card that lets us 
hook it up to a bunch of different boards.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091220/8532ae95/attachment.pgp 

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Dirk Behme]

On 20.12.2009 21:05, Mike Frysinger wrote:
> On Sunday 20 December 2009 14:30:35 Dirk Behme wrote:
>> For TI OMAP3 Beagle based Zippy expansion board from TinCanTools [1]
>> I'm currently looking into reusing spi based ENC28J60 network driver
>>
>> drivers/net/enc28j60.c
>>
>> It seems to me that it uses LPC2292 specific macros
>>
>> IO1CLR, IO1SET and IO1DIR
>>
>> These macros are defined in
>>
>> asm-arm/arch-lpc2292/lpc2292_registers.h
>
> this is why we didnt bother trying to get this part working on Blackfin boards
> under u-boot.  it works fine for us under Linux,

Similar situation with Beagle and Zippy.

> but the u-boot driver is a
> joke.

:(

>>    From enc28j60.c:
>>
>> ...
>> #define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
>> #define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
>> ...
>>
>> ...
>> /* configure GPIO */
>> (*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
>> (*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;
>>
>> /* CS and RESET active low */
>> PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
>> PUT32 (IO1SET, ENC_RESET);
>> ...
>>
>> Anybody with an idea how to move this code to some (LPC2292?) board
>> specific files to make enc28j60.c more generic to be able to reuse it
>> on other boards?
>
> unless the maintainers of the LPC2292 board are willing to help

Any idea who are the LPC2292 board maintainers? I couldn't find a 
LPC2292 entry in MAINTAINERS. Initial check in of enc28j60.c seems to 
be done by Peter Pearse.

>, i'd say just
> avoid the issue:
>   - rename/move this driver to indicate it is specific to LPC2292
>   - create a new driver based on the old one using the common SPI framework

Two additional questions:

- Which is the 'the common SPI framework'? Files?

- Just for correct understanding: We are talking about two issues 
here? The first issue is that enc28j60.c has board specific code, for 
e.g. setting GPIOs (as shown above)? And the second issue is that it 
doesn't use common SPI framework? Correct?

> once it starts using the common SPI framework, i should be able to easily help
> with testing on Blackfin boards.  we have a little addon card that lets us
> hook it up to a bunch of different boards.

Ok, I will have a look to it, but can't promise anything. At least it 
sounds like a good plan :) Any help will be appreciated, though ;)

Best regards

Dirk

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Mike Frysinger]

On Monday 21 December 2009 03:26:06 Dirk Behme wrote:
> - Which is the 'the common SPI framework'? Files?

include/spi.h ... just grep for files that include it and you'll find a bunch 
of examples in the tree.

> - Just for correct understanding: We are talking about two issues
> here? The first issue is that enc28j60.c has board specific code, for
> e.g. setting GPIOs (as shown above)? And the second issue is that it
> doesn't use common SPI framework? Correct?

when i read the driver, i couldnt tell how much the spi was bound to the 
board, but if things can be separated that way, then sure.

for the board init issue, a driver that has been converted to NET_MULTI means 
that it provides a hook for boards to call (enc28j60_register()).  then in the 
board-specific hook (board_eth_init()), you do all the board-specific stuff 
and then call enc28j60_register().

the README.driver.eth should explain it
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091221/47b867c4/attachment.pgp 

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Dirk Behme]

On 20.12.2009 21:05, Mike Frysinger wrote:
> On Sunday 20 December 2009 14:30:35 Dirk Behme wrote:
>> For TI OMAP3 Beagle based Zippy expansion board from TinCanTools [1]
>> I'm currently looking into reusing spi based ENC28J60 network driver
>>
>> drivers/net/enc28j60.c
>>
>> It seems to me that it uses LPC2292 specific macros
>>
>> IO1CLR, IO1SET and IO1DIR
>>
>> These macros are defined in
>>
>> asm-arm/arch-lpc2292/lpc2292_registers.h
>
> this is why we didnt bother trying to get this part working on Blackfin boards
> under u-boot.  it works fine for us under Linux, but the u-boot driver is a
> joke.
>
>>    From enc28j60.c:
>>
>> ...
>> #define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
>> #define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
>> ...
>>
>> ...
>> /* configure GPIO */
>> (*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
>> (*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;
>>
>> /* CS and RESET active low */
>> PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
>> PUT32 (IO1SET, ENC_RESET);
>> ...
>>
>> Anybody with an idea how to move this code to some (LPC2292?) board
>> specific files to make enc28j60.c more generic to be able to reuse it
>> on other boards?
>
> unless the maintainers of the LPC2292 board are willing to help, i'd say just
> avoid the issue:
>   - rename/move this driver to indicate it is specific to LPC2292
>   - create a new driver based on the old one using the common SPI framework
>
> once it starts using the common SPI framework, i should be able to easily help
> with testing on Blackfin boards.  we have a little addon card that lets us
> hook it up to a bunch of different boards.

I started to convert the enc28j60.c to common SPI framework. Do you 
like to have a look at attachment (and maybe test it?)?

It is compile tested only. And for the moment it just re-uses the 
existing driver. When we know that it basically works this way, doing 
it in a clean way as you describe above would be the next step. 
CONFIG_NET_MULTI is still missing, too.

In your config file you have to set and configure

#define CONFIG_xxx_SPI
#define CONFIG_ENC28J60
#define CONFIG_ENC28J60_SPI_BUS		0
#define CONFIG_ENC28J60_SPI_CS		0
#define CONFIG_ENC28J60_SPI_CLK		1000000
#define CONFIG_CMD_NET

for your board.

Opinions, ideas, proposals etc?

Best regards

Dirk

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: enc28j60_convert_to_spi_framework_patch.txt
Url: http://lists.denx.de/pipermail/u-boot/attachments/20091225/87b42fb5/attachment.txt 

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Mike Frysinger]

On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
> I started to convert the enc28j60.c to common SPI framework. Do you
> like to have a look at attachment (and maybe test it?)?
> 
> It is compile tested only. And for the moment it just re-uses the
> existing driver. When we know that it basically works this way, doing
> it in a clean way as you describe above would be the next step.
> CONFIG_NET_MULTI is still missing, too.

spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.  this 
isnt so much an "interrupts" issue as the process of claiming the bus 
reprograms the controller with the slave settings.

> In your config file you have to set and configure
> 
> #define CONFIG_xxx_SPI
> #define CONFIG_ENC28J60
> #define CONFIG_ENC28J60_SPI_BUS		0
> #define CONFIG_ENC28J60_SPI_CS		0
> #define CONFIG_ENC28J60_SPI_CLK		1000000
> #define CONFIG_CMD_NET
> 
> for your board.

this is ok with the current design, but broken for NET_MULTI.  when converted 
to NET_MULTI, the new enc28j60_register() function will take the spi settings 
as function arguments.  so the function would look something like:
int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int spi_cs, 
unsigned int max_hz, unsigned int mode);

and it'd be up to the board to call it with the settings it wants
-mike

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Dirk Behme]

On 26.12.2009 19:40, Mike Frysinger wrote:
> On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
>> I started to convert the enc28j60.c to common SPI framework. Do you
>> like to have a look at attachment (and maybe test it?)?
>>
>> It is compile tested only. And for the moment it just re-uses the
>> existing driver. When we know that it basically works this way, doing
>> it in a clean way as you describe above would be the next step.
>> CONFIG_NET_MULTI is still missing, too.
>
> spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.  this
> isnt so much an "interrupts" issue as the process of claiming the bus
> reprograms the controller with the slave settings.
>
>> In your config file you have to set and configure
>>
>> #define CONFIG_xxx_SPI
>> #define CONFIG_ENC28J60
>> #define CONFIG_ENC28J60_SPI_BUS		0
>> #define CONFIG_ENC28J60_SPI_CS		0
>> #define CONFIG_ENC28J60_SPI_CLK		1000000
>> #define CONFIG_CMD_NET
>>
>> for your board.
>
> this is ok with the current design, but broken for NET_MULTI.  when converted
> to NET_MULTI, the new enc28j60_register() function will take the spi settings
> as function arguments.  so the function would look something like:
> int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int spi_cs,
> unsigned int max_hz, unsigned int mode);
>
> and it'd be up to the board to call it with the settings it wants

Both changes, enc28j60_initialize() (NET_MULTI enabled) and 
spi_claim_bus/spi_release_bus done in below.

In the the board file I now have

int board_eth_init(bd_t *bis)
{
	int rc = 0;
#ifdef CONFIG_ENC28J60
	rc = enc28j60_initialize(bis,
				 CONFIG_ENC28J60_SPI_BUS,
				 CONFIG_ENC28J60_SPI_CS,
				 CONFIG_ENC28J60_SPI_CLK,
				 SPI_MODE_3);
#endif
	return rc;
}

Do you like to test? Any further comments?

As mentioned, when we know that it works this way, I will do a clean 
version.

Best regards

Dirk


Index: u-boot-main/drivers/net/enc28j60.c
===================================================================
--- u-boot-main.orig/drivers/net/enc28j60.c
+++ u-boot-main/drivers/net/enc28j60.c
@@ -18,8 +18,8 @@
  #include <config.h>
  #include <common.h>
  #include <net.h>
-#include <asm/arch/hardware.h>
-#include <asm/arch/spi.h>
+//#include <asm/arch/hardware.h>
+#include <spi.h>

  /*
   * Control Registers in Bank 0
@@ -284,10 +284,14 @@
  /* maximum frame length */
  #define ENC_MAX_FRM_LEN 1518

-#define enc_enable() PUT32(IO1CLR, ENC_SPI_SLAVE_CS)
-#define enc_disable() PUT32(IO1SET, ENC_SPI_SLAVE_CS)
-#define enc_cfg_spi() spi_set_cfg(0, 0, 0); spi_set_clock(8);
-
+#define enc_enable(x) spi_cs_activate(x);
+#define enc_disable(x) spi_cs_deactivate(x);
+#define spi_write(x) spi_w8r8(slave, x)
+#define spi_read() spi_w8r8(slave, 0)
+#define spi_lock() spi_claim_bus(slave)
+#define spi_unlock() spi_release_bus(slave)
+/* Use spi_setup_slave() instead of enc_cfg_spi() */
+#define enc_cfg_spi()

  static unsigned char encReadReg (unsigned char regNo);
  static void encWriteReg (unsigned char regNo, unsigned char data);
@@ -322,25 +326,26 @@ static unsigned char next_pointer_msb;
  static unsigned char buffer[ENC_MAX_FRM_LEN];
  static int rxResetCounter = 0;

+static struct spi_slave *slave;
+
  #define RX_RESET_COUNTER 1000;

 
/*-----------------------------------------------------------------------------
   * Always returns 0
   */
-int eth_init (bd_t * bis)
+int enc28j60_initialize(bd_t *bis, unsigned int spi_bus, unsigned int 
spi_cs,
+			unsigned int max_hz, unsigned int mode)
  {
  	unsigned char estatVal;
  	uchar enetaddr[6];

-	/* configure GPIO */
-	(*((volatile unsigned long *) IO1DIR)) |= ENC_SPI_SLAVE_CS;
-	(*((volatile unsigned long *) IO1DIR)) |= ENC_RESET;
-
-	/* CS and RESET active low */
-	PUT32 (IO1SET, ENC_SPI_SLAVE_CS);
-	PUT32 (IO1SET, ENC_RESET);
-
  	spi_init ();
+	if (!slave) {
+		slave = spi_setup_slave(spi_bus, spi_cs, max_hz, mode);
+		if (!slave)
+			return -1;
+	}
+	spi_claim_bus(slave);

  	/* taken from the Linux driver - dangerous stuff here! */
  	/* Wait for CLKRDY to become set (i.e., check that we can 
communicate with
@@ -592,17 +597,17 @@ static void encWriteReg (unsigned char r
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0x40 | regNo);	/* write in regNo */
  	spi_write (data);

-	enc_disable ();
-	enc_enable ();
+	enc_disable (slave);
+	enc_enable (slave);

  	spi_write (0x1f);	/* write reg 0x1f */

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();
  }

@@ -615,17 +620,17 @@ static void encWriteRegRetry (unsigned c

  	for (i = 0; i < c; i++) {
  		enc_cfg_spi ();
-		enc_enable ();
+		enc_enable (slave);

  		spi_write (0x40 | regNo);	/* write in regNo */
  		spi_write (data);

-		enc_disable ();
-		enc_enable ();
+		enc_disable (slave);
+		enc_enable (slave);

  		spi_write (0x1f);	/* write reg 0x1f */

-		enc_disable ();
+		enc_disable (slave);

  		spi_unlock ();	/* we must unlock spi first */

@@ -649,14 +654,14 @@ static unsigned char encReadReg (unsigne

  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0x1f);	/* read reg 0x1f */

  	bank = spi_read () & 0x3;

-	enc_disable ();
-	enc_enable ();
+	enc_disable (slave);
+	enc_enable (slave);

  	spi_write (regNo);
  	rxByte = spi_read ();
@@ -668,7 +673,7 @@ static unsigned char encReadReg (unsigne
  		rxByte = spi_read ();
  	}

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();

  	return rxByte;
@@ -678,7 +683,7 @@ static void encReadBuff (unsigned short
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0x20 | 0x1a);	/* read buffer memory */

@@ -689,7 +694,7 @@ static void encReadBuff (unsigned short
  			spi_write (0);
  	}

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();
  }

@@ -697,7 +702,7 @@ static void encWriteBuff (unsigned short
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0x60 | 0x1a);	/* write buffer memory */

@@ -706,7 +711,7 @@ static void encWriteBuff (unsigned short
  	while (length--)
  		spi_write (*pBuff++);

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();
  }

@@ -714,12 +719,12 @@ static void encBitSet (unsigned char reg
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0x80 | regNo);	/* bit field set */
  	spi_write (data);

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();
  }

@@ -727,12 +732,12 @@ static void encBitClr (unsigned char reg
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0xA0 | regNo);	/* bit field clear */
  	spi_write (data);

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();
  }

@@ -740,11 +745,11 @@ static void encReset (void)
  {
  	spi_lock ();
  	enc_cfg_spi ();
-	enc_enable ();
+	enc_enable (slave);

  	spi_write (0xff);	/* soft reset */

-	enc_disable ();
+	enc_disable (slave);
  	spi_unlock ();

  	/* sleep 1 ms. See errata pt. 2 */
Index: u-boot-main/include/netdev.h
===================================================================
--- u-boot-main.orig/include/netdev.h
+++ u-boot-main/include/netdev.h
@@ -49,6 +49,8 @@ int davinci_emac_initialize(void);
  int dnet_eth_initialize(int id, void *regs, unsigned int phy_addr);
  int e1000_initialize(bd_t *bis);
  int eepro100_initialize(bd_t *bis);
+int enc28j60_initialize(bd_t *bis, unsigned int spi_bus, unsigned int 
spi_cs,
+			unsigned int max_hz, unsigned int mode);
  int eth_3com_initialize (bd_t * bis);
  int fec_initialize (bd_t *bis);
  int fecmxc_initialize (bd_t *bis);

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Ben Warren]

Hi Dirk,

On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behme <dirk.behme@googlemail.com>wrote:

> On 26.12.2009 19:40, Mike Frysinger wrote:
> > On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
> >> I started to convert the enc28j60.c to common SPI framework. Do you
> >> like to have a look at attachment (and maybe test it?)?
> >>
> >> It is compile tested only. And for the moment it just re-uses the
> >> existing driver. When we know that it basically works this way, doing
> >> it in a clean way as you describe above would be the next step.
> >> CONFIG_NET_MULTI is still missing, too.
> >
> > spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.
>  this
> > isnt so much an "interrupts" issue as the process of claiming the bus
> > reprograms the controller with the slave settings.
> >
> >> In your config file you have to set and configure
> >>
> >> #define CONFIG_xxx_SPI
> >> #define CONFIG_ENC28J60
> >> #define CONFIG_ENC28J60_SPI_BUS              0
> >> #define CONFIG_ENC28J60_SPI_CS               0
> >> #define CONFIG_ENC28J60_SPI_CLK              1000000
> >> #define CONFIG_CMD_NET
> >>
> >> for your board.
> >
> > this is ok with the current design, but broken for NET_MULTI.  when
> converted
> > to NET_MULTI, the new enc28j60_register() function will take the spi
> settings
> > as function arguments.  so the function would look something like:
> > int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int
> spi_cs,
> > unsigned int max_hz, unsigned int mode);
> >
> > and it'd be up to the board to call it with the settings it wants
>
> Both changes, enc28j60_initialize() (NET_MULTI enabled) and
> spi_claim_bus/spi_release_bus done in below.
>
> In the the board file I now have
>
> int board_eth_init(bd_t *bis)
> {
>        int rc = 0;
> #ifdef CONFIG_ENC28J60
>        rc = enc28j60_initialize(bis,
>                                 CONFIG_ENC28J60_SPI_BUS,
>                                 CONFIG_ENC28J60_SPI_CS,
>                                 CONFIG_ENC28J60_SPI_CLK,
>                                 SPI_MODE_3);
> #endif
>        return rc;
> }
>
> This is the right way to do it.  Thanks for taking on this task.  One
comment is that I'm not sure you need to pass 'bis' to the initialize()
function.


> Do you like to test? Any further comments?
>
I'm sure you know that converting to CONFIG_NET_MULTI involves a lot more
than renaming the initialize() functions.  Looking forward to the full
version.  I wish I had hardware to help you, but will be happy to review
once you're ready.


> As mentioned, when we know that it works this way, I will do a clean
> version.
>
> Best regards
>
> Dirk
>
> regards,
Ben

>
>

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Dirk Behme]

On 27.12.2009 16:32, Ben Warren wrote:
> Hi Dirk,
>
> On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behme<dirk.behme@googlemail.com>wrote:
>
>> On 26.12.2009 19:40, Mike Frysinger wrote:
>>> On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
>>>> I started to convert the enc28j60.c to common SPI framework. Do you
>>>> like to have a look at attachment (and maybe test it?)?
>>>>
>>>> It is compile tested only. And for the moment it just re-uses the
>>>> existing driver. When we know that it basically works this way, doing
>>>> it in a clean way as you describe above would be the next step.
>>>> CONFIG_NET_MULTI is still missing, too.
>>>
>>> spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.
>>   this
>>> isnt so much an "interrupts" issue as the process of claiming the bus
>>> reprograms the controller with the slave settings.
>>>
>>>> In your config file you have to set and configure
>>>>
>>>> #define CONFIG_xxx_SPI
>>>> #define CONFIG_ENC28J60
>>>> #define CONFIG_ENC28J60_SPI_BUS              0
>>>> #define CONFIG_ENC28J60_SPI_CS               0
>>>> #define CONFIG_ENC28J60_SPI_CLK              1000000
>>>> #define CONFIG_CMD_NET
>>>>
>>>> for your board.
>>>
>>> this is ok with the current design, but broken for NET_MULTI.  when
>> converted
>>> to NET_MULTI, the new enc28j60_register() function will take the spi
>> settings
>>> as function arguments.  so the function would look something like:
>>> int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int
>> spi_cs,
>>> unsigned int max_hz, unsigned int mode);
>>>
>>> and it'd be up to the board to call it with the settings it wants
>>
>> Both changes, enc28j60_initialize() (NET_MULTI enabled) and
>> spi_claim_bus/spi_release_bus done in below.
>>
>> In the the board file I now have
>>
>> int board_eth_init(bd_t *bis)
>> {
>>         int rc = 0;
>> #ifdef CONFIG_ENC28J60
>>         rc = enc28j60_initialize(bis,
>>                                  CONFIG_ENC28J60_SPI_BUS,
>>                                  CONFIG_ENC28J60_SPI_CS,
>>                                  CONFIG_ENC28J60_SPI_CLK,
>>                                  SPI_MODE_3);
>> #endif
>>         return rc;
>> }
>>
>> This is the right way to do it.  Thanks for taking on this task.  One
> comment is that I'm not sure you need to pass 'bis' to the initialize()
> function.

 From functionality point of view it's not needed. The API was 
proposed by Mike this way. But yes, 'bis' can be removed.

>> Do you like to test? Any further comments?
>>
> I'm sure you know that converting to CONFIG_NET_MULTI involves a lot more
> than renaming the initialize() functions.

Could you give some examples or hints, what you like to see? Just in 
case I missed the details ;)

> Looking forward to the full
> version.

First I'd like to hear if it basically works for someone. If not, it 
makes no sense to go on ;)

> I wish I had hardware to help you, but will be happy to review
> once you're ready.

Mike mentioned that he eventually could test it on Blackfin boards.

Many thanks for review,

Dirk

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Mike Frysinger]

On Sunday 27 December 2009 13:55:02 Dirk Behme wrote:
> On 27.12.2009 16:32, Ben Warren wrote:
> > I'm sure you know that converting to CONFIG_NET_MULTI involves a lot more
> > than renaming the initialize() functions.
> 
> Could you give some examples or hints, what you like to see? Just in
> case I missed the details ;)

look at the doc/README.drivers.eth for how to write a net multi driver

> > I wish I had hardware to help you, but will be happy to review
> > once you're ready.
> 
> Mike mentioned that he eventually could test it on Blackfin boards.

i'll try and find some time to do this in the next week or two
-mike

[U-Boot] Remove board specific code from ENC28J60 network driver?

[Ben Warren]

Hi Dirk,

On Sun, Dec 27, 2009 at 10:55 AM, Dirk Behme <dirk.behme@googlemail.com>wrote:

> On 27.12.2009 16:32, Ben Warren wrote:
>
>> Hi Dirk,
>>
>> On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behme<dirk.behme@googlemail.com
>> >wrote:
>>
>>  On 26.12.2009 19:40, Mike Frysinger wrote:
>>>
>>>> On Friday 25 December 2009 13:57:55 Dirk Behme wrote:
>>>>
>>>>> I started to convert the enc28j60.c to common SPI framework. Do you
>>>>> like to have a look at attachment (and maybe test it?)?
>>>>>
>>>>> It is compile tested only. And for the moment it just re-uses the
>>>>> existing driver. When we know that it basically works this way, doing
>>>>> it in a clean way as you describe above would be the next step.
>>>>> CONFIG_NET_MULTI is still missing, too.
>>>>>
>>>>
>>>> spi_lock/spi_unlock should redirect to spi_claim_bus/spi_release_bus.
>>>>
>>>  this
>>>
>>>> isnt so much an "interrupts" issue as the process of claiming the bus
>>>> reprograms the controller with the slave settings.
>>>>
>>>>  In your config file you have to set and configure
>>>>>
>>>>> #define CONFIG_xxx_SPI
>>>>> #define CONFIG_ENC28J60
>>>>> #define CONFIG_ENC28J60_SPI_BUS              0
>>>>> #define CONFIG_ENC28J60_SPI_CS               0
>>>>> #define CONFIG_ENC28J60_SPI_CLK              1000000
>>>>> #define CONFIG_CMD_NET
>>>>>
>>>>> for your board.
>>>>>
>>>>
>>>> this is ok with the current design, but broken for NET_MULTI.  when
>>>>
>>> converted
>>>
>>>> to NET_MULTI, the new enc28j60_register() function will take the spi
>>>>
>>> settings
>>>
>>>> as function arguments.  so the function would look something like:
>>>> int enc28j60_register(bd_t *bis, unsigned int spi_bus, unsigned int
>>>>
>>> spi_cs,
>>>
>>>> unsigned int max_hz, unsigned int mode);
>>>>
>>>> and it'd be up to the board to call it with the settings it wants
>>>>
>>>
>>> Both changes, enc28j60_initialize() (NET_MULTI enabled) and
>>> spi_claim_bus/spi_release_bus done in below.
>>>
>>> In the the board file I now have
>>>
>>> int board_eth_init(bd_t *bis)
>>> {
>>>        int rc = 0;
>>> #ifdef CONFIG_ENC28J60
>>>        rc = enc28j60_initialize(bis,
>>>                                 CONFIG_ENC28J60_SPI_BUS,
>>>                                 CONFIG_ENC28J60_SPI_CS,
>>>                                 CONFIG_ENC28J60_SPI_CLK,
>>>                                 SPI_MODE_3);
>>> #endif
>>>        return rc;
>>> }
>>>
>>> This is the right way to do it.  Thanks for taking on this task.  One
>>>
>> comment is that I'm not sure you need to pass 'bis' to the initialize()
>> function.
>>
>
> From functionality point of view it's not needed. The API was proposed by
> Mike this way. But yes, 'bis' can be removed.
>
>
>  Do you like to test? Any further comments?
>>>
>>>  I'm sure you know that converting to CONFIG_NET_MULTI involves a lot
>> more
>> than renaming the initialize() functions.
>>
>
> Could you give some examples or hints, what you like to see? Just in case I
> missed the details ;)
>
> As Mike mentioned, there's documentation.  If you'd like a simple,
properly-done driver as a reference, have a look at drivers/net/cs8900.c

>
>  Looking forward to the full
>> version.
>>
>
> First I'd like to hear if it basically works for someone. If not, it makes
> no sense to go on ;)
>
>
>  I wish I had hardware to help you, but will be happy to review
>> once you're ready.
>>
>
> Mike mentioned that he eventually could test it on Blackfin boards.
>
> Many thanks for review,
>
> Dirk
>
regards,
Ben