[U-Boot] Remove board specific code from ENC28J60 network driver?
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&...
Dirk,
On Sun, Dec 20, 2009 at 11:30 AM, Dirk Behme dirk.behme@googlemail.comwrote:
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
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
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
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
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
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
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);
Hi Dirk,
On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behme dirk.behme@googlemail.comwrote:
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
On 27.12.2009 16:32, Ben Warren wrote:
Hi Dirk,
On Sat, Dec 26, 2009 at 11:59 PM, Dirk Behmedirk.behme@googlemail.comwrote:
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
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
Hi Dirk,
On Sun, Dec 27, 2009 at 10:55 AM, Dirk Behme dirk.behme@googlemail.comwrote:
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
participants (3)
-
Ben Warren -
Dirk Behme -
Mike Frysinger