NET: add ENC28J60 driver using SPI framework
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
From: Reinhard Meyer <[email protected]>
--- drivers/net/Makefile | 1 + drivers/net/enc28j60.c | 650 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/net/enc28j60.h | 217 ++++++++++++++++ include/netdev.h | 1 + 4 files changed, 869 insertions(+), 0 deletions(-) create mode 100644 drivers/net/enc28j60.c create mode 100644 drivers/net/enc28j60.h
drivers/net/Makefile +1 -0
@@ -39,6 +39,7 @@ COBJS-$(CONFIG_DRIVER_DM9000) += dm9000x.o COBJS-$(CONFIG_DNET) += dnet.o COBJS-$(CONFIG_E1000) += e1000.o COBJS-$(CONFIG_EEPRO100) += eepro100.o +COBJS-$(CONFIG_ENC28J60) += enc28j60.o COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o COBJS-$(CONFIG_EP93XX) += ep93xx_eth.o COBJS-$(CONFIG_ETHOC) += ethoc.o
drivers/net/enc28j60.c new file +650 -0
650 lines not on this page · raw message
drivers/net/enc28j60.h new file +217 -0
217 lines not on this page · raw message
include/netdev.h +1 -0
@@ -54,6 +54,7 @@ int designware_initialize(u32 id, ulong base_addr, u32 phy_addr);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 (int id);int ep93xx_eth_initialize (u8 dev_num,int base_addr);int ethoc_initialize (u8 dev_num,int base_addr);int eth_3com_initialize (bd_t * bis);
Signature · 1 line
1.5.6.5
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 3 lines · On Wednesday, August 25, 2010 08:47:39 Reinhard Meyer wrote:
From: Reinhard Meyer <[email protected]>
Signed-off-by Reinhard Meyer <[email protected]>
these two fields should match (e-mail)
also, this patch seems to depend on some other change not in mainline or Ben's net tree ...
drivers/net/enc28j60.c line 22
Quoted 2 lines
drivers/net/enc28j60.c new file +2
-0+#include <config.h> +#include <common.h>
the common.h header takes care of config.h already
Quoted 1 line
+#include <
asm/arch/hardware.h>
err, what ?
Quoted 1 line
+#define
ENC28J60_DRIVERNAME"enc"
this is used in only one place, so no point in a define
Quoted 1 line
+#define
FAILSAFE_VALUE5000
this isnt used anywhere
drivers/net/enc28j60.c line 61
Quoted 2 lines
drivers/net/enc28j60.c new file +2
-0+static u8 enc_r8 (u16 regNo); +static void enc_rbuf (u16 length,u8 *pBuff);
this whole file needs to be checked for function style. no space after the function name.
also, drop the camel case and hungarian notation. use "reg" and "buff".
drivers/net/enc28j60.c line 76
Quoted 2 lines
drivers/net/enc28j60.c new file +2
-0+/* NetReceive() requires the packet to be word aligned! */ +static u8 packet[ENC_MAX_FRM_LEN] __attribute__ ((aligned(4)));
why do you need to declare your own buffer ? the common code already sets up one for you and it uses the standard 1518 size (PKTSIZE).
Quoted 1 line
+int
enc28j60_init(structeth_device*dev,bd_t* bis)
all of the enc28j60 funcs that go into eth_device dont need to be public. so add static to the init/halt/recv/send funcs.
also, it's "bd_t *bis"
Quoted 2 lines
(quoted diff) +1
-0+ slave = spi_setup_slave(ENC_SPI_BUS, ENC_CS_NUM, ENC_SPI_CLOCK,
SPI_MODE_0);
pass the bus/cs/speed/mode in to the initialize function and store it in the per-device state. then you can work fine with multiple enc28j60 devices in one board.
along those lines, all of the local enc funcs should be changed to take the private state and operate off that instead of random global variables.
drivers/net/enc28j60.c line 121
Quoted 1 line
drivers/net/enc28j60.c new file +1
-0+spi_claim_bus (slave);
missing check on the return value
drivers/net/enc28j60.c line 130
Quoted 4 lines
drivers/net/enc28j60.c new file +4
-0+do + { + estatVal =enc_r8 (CTL_REG_ESTAT ); + }while ((estatVal &0x08 ) || (~estatVal &ENC_ESTAT_CLKRDY ));
the bit inverting ("~") should be on the mask so it's done at compile time instead of forcing the board to do it at runtime
drivers/net/enc28j60.c line 139
Quoted 6 lines
drivers/net/enc28j60.c new file +6
-0+#ifdef CONFIG_NET_MULTI +memcpy (enetaddr,eth_get_dev ()->enetaddr,6 ); +#else +eth_getenv_enetaddr ("ethaddr" , enetaddr); +#endif +enc_init (enetaddr);
this should be one line:
enc_init(dev->enetaddr);
also, nowhere in this init func can i see error checking that the enc28j60 device is actually out there. the enc_init() should return an error if the expected phy's/etc... dont match, and this init func should key off that.
drivers/net/enc28j60.c line 475
Quoted 4 lines
drivers/net/enc28j60.c new file +4
-0+static void enc_init (u8 *pEthAddr) +{ +u16 phid1 =0 ; +u16 phid2 =0 ;
these are always set up below, so no point in setting them to 0
drivers/net/enc28j60.c line 625
Quoted 3 lines
drivers/net/enc28j60.c new file +2
-0+static void phy_write (u8 addr,u16 data) + while((enc_r8(CTL_REG_MISTAT) & ENC_MISTAT_BUSY) != 0) {
missing a space after that "while"
drivers/net/enc28j60.c line 647
Quoted 1 line
drivers/net/enc28j60.c new file +1
-0+sprintf (dev->name,"%s%hu" ,ENC28J60_DRIVERNAME , dev_num);
once this func is fixed to take the spi bus/cs, the name can be based on that instead of an arbitrary integer.
"enc%i.%i", bus, cs
drivers/net/enc28j60.h
Quoted 2 lines
drivers/net/enc28j60.h new file +0
-0
this header is missing ifdef protection against multiple inclusion
include/netdev.h
Quoted 4 lines
include/netdev.h +1
-0+int enc28j60_initialize (int id);
this doesnt match the prototype in the source. you should have the .c file include netdev.h so that desyncs are caught & prevented in the future. -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/20100825/87dd332f/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Dear Mike Frysinger,
Quoted 1 line
Quoted 1 line
From: Reinhard Meyer<[email protected]>
Give me a tip how I can change that info. The first patch to this was committed while I used <info>. Squashing other patches into it does not change that. rebase -i, edit, --amend does not present that line for edit. The only idea I have now is making an empty commit and squashing that before the original patch...
Quoted 2 lines
also, this patch seems to depend on some other change not in mainline or Ben's net tree ...
Should have mentioned that the "move enc28j60 to sidetrack" patch is to be applied first.
Quoted 5 lines
drivers/net/enc28j60.c line 61
Quoted 2 lines
drivers/net/enc28j60.c new file +2
-0+static u8 enc_r8 (u16 regNo); +static void enc_rbuf (u16 length,u8 *pBuff);this whole file needs to be checked for function style. no space after the function name.
That's what happens if one reworks an existing driver. I'll recheck all this.
Quoted 2 lines
why do you need to declare your own buffer ? the common code already sets up one for you and it uses the standard 1518 size (PKTSIZE).
Ok, better. Just hint me on its name, browsing the common code none did catch my eye:)
Quoted 3 lines
pass the bus/cs/speed/mode in to the initialize function and store it in the per-device state. then you can work fine with multiple enc28j60 devices in one board.
<joke>
Ok, I was already planning to build a 16 port hub using 16 such devices ;) </joke>
But generalization is good. I might end up using two, actually.
Quoted 3 lines
also, nowhere in this init func can i see error checking that the enc28j60 device is actually out there. the
enc_init()should return an error if the expected phy's/etc... dont match, and this init func should key off that.
I'd like to even have the phy start auto-negotiation at that moment, I'm just not sure that's ok. I won't wait for it to complete, of course, just trigger it.
Quoted 3 lines
once this func is fixed to take the spi bus/cs, the name can be based on that instead of an arbitrary integer.
"enc%i.%i", bus, cs
setenv ethact enc1.13 - one can get used to that ;)
Reinhard
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 7 lines · On Thursday, August 26, 2010 02:19:07 Reinhard Meyer wrote:
Quoted 1 line
Quoted 1 line
From: Reinhard Meyer<[email protected]>
Give me a tip how I can change that info. The first patch to this was committed while I used <info>. Squashing other patches into it does not change that. rebase -i, edit, --amend does not present that line for edit. The only idea I have now is making an empty commit and squashing that before the original patch...
use the --author option to the commit sub command
Quoted 5 lines
Quoted 2 lines
why do you need to declare your own buffer ? the common code already sets up one for you and it uses the standard 1518 size (PKTSIZE).
Ok, better. Just hint me on its name, browsing the common code none did catch my eye:)
NetRxPackets and friends
-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/20100826/933d07b3/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Dear Mike Frysinger,
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
From: Reinhard Meyer<[email protected]>
Give me a tip how I can change that info. The first patch to this was committed while I used<info>. Squashing other patches into it does not change that. rebase -i, edit, --amend does not present that line for edit. The only idea I have now is making an empty commit and squashing that before the original patch...
use the --author option to the commit sub command
Hmm, as I said above, the first commit was made while I had the <info> address in use. All new commits use the <u-boot> address anyway. But when I squash new commits onto the first one, the old address prevails...
I'll try "rebase -i", "edit", reset HEAD^, and a new commit then. That might work.
Reinhard
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 8 lines · On Thursday, August 26, 2010 02:41:56 Reinhard Meyer wrote:
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
From: Reinhard Meyer<[email protected]>
Give me a tip how I can change that info. The first patch to this was committed while I used<info>. Squashing other patches into it does not change that. rebase -i, edit, --amend does not present that line for edit. The only idea I have now is making an empty commit and squashing that before the original patch...
Quoted 6 lines
Quoted 1 line
use the --author option to the commit sub command
Hmm, as I said above, the first commit was made while I had the <info> address in use. All new commits use the <u-boot> address anyway. But when I squash new commits onto the first one, the old address prevails...
there is no "field" to edit in the commit message. the "From:" is generated by the author information in the commit. the only way to change it is to amend the commit in question with --author.
-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/20100826/172ca580/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
In reply to Mike FrysingerDear Mike Frysinger,
Quoted 6 lines
pass the bus/cs/speed/mode in to the initialize function and store it in the per-device state. then you can work fine with multiple enc28j60 devices in one board.
along those lines, all of the local enc funcs should be changed to take the private state and operate off that instead of random global variables.
Something like this?
typedef struct enc_device {
struct eth_device netdev;
struct spi_slave *slave;
u16 next_pointer;
int rx_reset_counter;
u8 bank; /* current bank in enc28j60 */
} enc_dev_t;#define to_enc(_nd) container_of(_nd, struct enc_device, netdev)
...
static u8 enc_r8(enc_dev_t *, u16 reg);
static u16 enc_r16(enc_dev_t *, u16 reg);
static void enc_w8(enc_dev_t *, u16 reg, u8 data); static void enc_w16(enc_dev_t *, u16 reg, u16 data); static void enc_w8_retry(enc_dev_t *, u16 reg, u8 data, int c);
I agree that's more perfect but kind of overkill, too; assuming that only one enc can be active at any given time;)
Best Regards,
Reinhard
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 1 line · On Friday, September 03, 2010 13:50:28 Reinhard Meyer wrote:
Something like this?
yes
Quoted 2 lines
I agree that's more perfect but kind of overkill, too; assuming that only one enc can be active at any given time;)
yes and no. the spi bus/cs/mode/speed is established at initialize() time, not init(), so you'd need to store that per-instance information somewhere. and how the discussion about netconsole handling falls out (not calling init/halt after every transaction), this base assumption may not be valid. -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/20100903/3300012f/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 5 lines · Mike Frysinger
yes and no. the spi bus/cs/mode/speed is established at
initialize()time, notinit(), so you'd need to store that per-instance information somewhere. and how the discussion about netconsole handling falls out (not calling init/halt after every transaction), this base assumption may not be valid. -mike
/*
* This is the only exported function.
*
* It may be called several times with different bus:cs combinations. */
int enc_initialize(int bus, int cs, int speed)
{
enc_dev_t *enc;
enc = malloc(sizeof(*enc));
if (!enc) {
return -1;
}
memset(enc, 0, sizeof(*enc));
enc->slave = spi_setup_slave(bus, cs, speed, SPI_MODE_0);
if (!enc->slave) {
free(enc);
return -1;
}
enc->netdev.init = enc_init;
enc->netdev.halt = enc_halt;
enc->netdev.send = enc_send;
enc->netdev.recv = enc_recv;
sprintf(enc->netdev.name, "enc%i.%i", bus, cs);
eth_register(&(enc->netdev));
return 0;
}(not compiled yet)
not sure, however, if calling spi_setup_slave() is ok at this point.
Reinhard
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 1 line · On Friday, September 03, 2010 16:31:47 Reinhard Meyer wrote:
int
enc_initialize(int bus, int cs, int speed)
should take the same parameters (including type) as spi_setup_slave
Quoted 1 line
eth_register(&(enc->netdev));
no need for the () before the &
Quoted 1 line
not sure, however, if calling
spi_setup_slave()is ok at this point.
i believe the func should only doing validation on the arguments. it shouldnt need to talk to any actual hardware. maybe this needs codifying in the API documentation.
-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/20100903/3bb55261/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 5 lines · Mike Frysinger
Quoted 1 line
not sure, however, if calling
spi_setup_slave()is ok at this point.i believe the func should only doing validation on the arguments. it shouldnt need to talk to any actual hardware. maybe this needs codifying in the API documentation.
In atmel_spi.c it sets up the csr[indexed by cs=0..3] register with clock, phase and polarity.
That's harmless, but might need some rethinking there.
Reinhard
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 10 lines · On Friday, September 03, 2010 17:23:43 Reinhard Meyer wrote:
Quoted 5 lines · Mike Frysinger
Quoted 1 line
not sure, however, if calling
spi_setup_slave()is ok at this point.i believe the func should only doing validation on the arguments. it shouldnt need to talk to any actual hardware. maybe this needs codifying in the API documentation.
In
atmel_spi.cit sets up the csr[indexed by cs=0..3] register with clock, phase and polarity.
That's harmless, but might need some rethinking there.
i'm not familiar with the Atmel SPI controller, but my gut reaction is that register values should be calculated in spi_setup_slave(), stored in the internal xxx_spi_slave struct, and then written at spi_claim_bus() time. -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/20100903/dd926dab/attachment.pgp
PATCH 1/1 NET: add ENC28J60 driver using SPI framework
Quoted 8 lines · Mike Frysinger
Quoted 7 lines · On Friday, September 03, 2010 17:23:43 Reinhard Meyer wrote:
Quoted 5 lines · Mike Frysinger
Quoted 1 line
not sure, however, if calling
spi_setup_slave()is ok at this point.i believe the func should only doing validation on the arguments. it shouldnt need to talk to any actual hardware. maybe this needs codifying in the API documentation.
Quoted 8 lines
Quoted 3 lines
In
atmel_spi.cit sets up the csr[indexed by cs=0..3] register with clock, phase and polarity.
That's harmless, but might need some rethinking there.i'm not familiar with the Atmel SPI controller, but my gut reaction is that register values should be calculated in
spi_setup_slave(), stored in the internalxxx_spi_slavestruct, and then written atspi_claim_bus()time. -mike
Well, the SPI hardware can store the settings for 4 chip selects, and quite automagically switch between them, including programmable switch times. When using DMA it would be possible to setup transfers to/from different chip selects without software doing the switches, ex:
15 words of 13 bits using 199kHz and mode 3 via CS2,
2 words of 8 bits using 10MHz and mode 0 via CS3, etc...
All this, of course, is not needed in u-boot and currently only half used anyway. Therefore I tend to rework the atmel_spi.c not to make use of that feature at all.
For codig the ENC driver I will just postulate its save to setup the slave at initialize time.
Reinhard