net/eth_device: keep index inside each device

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Jean-Christophe,

Quoted 1 line · Jean-Christophe PLAGNIOL-VILLARD

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

I already posted a patch that does this and asked for help testing. It is available in the net/next repo. If you find it to be inadequate, please provide patches to make it work.

Quoted 3 lines · Ben Warren

Jean-Christophe,
Jean-Christophe PLAGNIOL-VILLARD wrote:

Quoted 6 lines

Quoted 2 lines

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

I already posted a patch that does this and asked for help testing. It is available in the net/next repo. If you find it to be inadequate, please provide patches to make it work.

I've also update the mac support and allow multi chip support does you pacth do the same?

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

drivers/net/smc91111.c +1 -1

-#undef USE_32_BIT
+#define is_use_32bit(x) (0)
 #endif

drivers/net/smc91111.c +2 -2

-static inline word SMC_inw(dword offset)
+static inline word SMC_io_inw(struct smc91111_device *smc, dword offset)
 {
 	word v;
-	v = *((volatile word*)(SMC_BASE_ADDRESS+offset));
+	v = *((volatile word*)(smc->regs+offset));
 	barrier(); *(volatile u32*)(0xc0000000);
 	return v;

drivers/net/smc91111.c +2 -1

@@ -264,18 +234,22 @@ static inline byte SMC_inb(dword offset)
 	return (offset & 1) ? (byte)(_w >> 8) : (byte)(_w);
 }
 
-static inline void SMC_outb(byte value, dword offset)
+static inline void SMC_io_outb(struct smc91111_device *smc, byte value,
+			       dword offset)
 {
 	word  _w;
 

drivers/net/smc91111.h +4 -0

index 967addd..b2ed6a5 100644
@@ -72,6 +72,10 @@ typedef unsigned long int		dword;
 
 /* Because of bank switching, the LAN91xxx uses only 16 I/O ports */
 
+#ifndef SMC_BASE_ADDRESS
+#define SMC_BASE_ADDRESS smc->regs
+#endif
+
 #define	SMC_IO_EXTENT	16
 
 #ifdef CONFIG_PXA250

drivers/net/smc91111.h +0 -0

 include/configs/ADNPESC1.h       |    1 +

 include/configs/DK1C20.h         |    1 +
 include/configs/DK1S10.h         |    1 +
 include/configs/EP1C20.h         |    1 +
 include/configs/EP1S10.h         |    1 +
 include/configs/EP1S40.h         |    1 +
 include/configs/MigoR.h          |    1 +
 include/configs/PK1C20.h         |    1 +
 include/configs/bf533-ezkit.h    |    1 +
 include/configs/bf533-stamp.h    |    1 +
 include/configs/bf538f-ezkit.h   |    1 +
 include/configs/bf561-ezkit.h    |    1 +
 include/configs/blackstamp.h     |    1 +
 include/configs/cerf250.h        |    1 +
 include/configs/cm-bf533.h       |    1 +
 include/configs/cm-bf561.h       |    1 +
 include/configs/cradle.h         |    1 +
 include/configs/delta.h          |    1 +
 include/configs/dnp1110.h        |    1 +
 include/configs/gr_cpci_ax2000.h |    1 +
 include/configs/gr_ep2s60.h      |    1 +
 include/configs/innokom.h        |    1 +
 include/configs/integratorcp.h   |    1 +
 include/configs/logodl.h         |    1 +
 include/configs/lpd7a400-10.h    |    1 +
 include/configs/lpd7a404-10.h    |    1 +
 include/configs/ms7722se.h       |    1 +
 include/configs/netstar.h        |    1 +

 include/configs/nhk8815.h        |    1 +
 include/configs/pxa255_idp.h     |    1 +
 include/configs/versatile.h      |    1 +
 include/configs/voiceblue.h      |    1 +
 include/configs/xaeniax.h        |    1 +
 include/configs/xm250.h          |    1 +
 include/configs/xsengine.h       |    1 +
 include/configs/zylonite.h       |    1 +

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 1 line · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

...

drivers/net/smc91111.c static const char version[] = · line 139

Quoted 3 lines

#ifdef CONFIG_SMC_USE_32_BIT

drivers/net/smc91111.c +1 -1

-#define USE_32_BIT  1
+#define is_use_32bit(x) (x->use_32bit)

Does switching from a compile time check to a run time check not cause an avoidable growth of the memory footprint?

How much bigger is the new code?

Quoted 4 lines

drivers/net/smc91111.c +1 -1

-#undef USE_32_BIT
+#define is_use_32bit(x) (0)
 #endif

...

Quoted 10 lines

drivers/net/smc91111.c +2 -2

-static inline word SMC_inw(dword offset)
+static inline word SMC_io_inw(struct smc91111_device *smc, dword offset)
 {
 	word v;
-	v = *((volatile word*)(SMC_BASE_ADDRESS+offset));
+	v = *((volatile word*)(smc->regs+offset));
 	barrier(); *(volatile u32*)(0xc0000000);
 	return v;
 }

drivers/net/smc91111.c int get_rom_mac(uchar *v_rom_mac); · line 227

Quoted 8 lines

drivers/net/smc91111.c +3 -2

-static inline void SMC_outw(word value, dword offset)
+static inline void SMC_io_outw(struct smc91111_device *smc, word value,
+			       dword offset)
 {
-	*((volatile word*)(SMC_BASE_ADDRESS+offset)) = value;
+	*((volatile word*)(smc->regs+offset)) = value;
 	barrier(); *(volatile u32*)(0xc0000000);
 }

Please use proper I/O accessor functions here.

drivers/net/smc91111.c static int smc_rcv() · line 981

Quoted 6 lines

drivers/net/smc91111.c +2 -1

-static int smc_close()
+static void smc_halt(struct eth_device *netdev)
 {
+	struct smc91111_device *smc = to_smc91111(netdev);
 	PRINTK2("%s: smc_close\n", SMC_DEV_NAME);

You should also adapt the debug messages to the changed function names.

drivers/net/smc91111.c int get_rom_mac (uchar *v_rom_mac) · line 1575

Quoted 5 lines

drivers/net/smc91111.c +5 -0

+#ifdef CONFIG_SMC_USE_32_BIT
+#define USE_32BIT 1
+#else
+#define USE_32BIT 0
+#endif

Above you get rid of the USE_32BIT stuff; here you re-introduce it. Why?

Quoted 28 lines

 include/configs/ADNPESC1.h       |    1 +
 include/configs/DK1C20.h         |    1 +
 include/configs/DK1S10.h         |    1 +
 include/configs/EP1C20.h         |    1 +
 include/configs/EP1S10.h         |    1 +
 include/configs/EP1S40.h         |    1 +
 include/configs/MigoR.h          |    1 +
 include/configs/PK1C20.h         |    1 +
 include/configs/bf533-ezkit.h    |    1 +
 include/configs/bf533-stamp.h    |    1 +
 include/configs/bf538f-ezkit.h   |    1 +
 include/configs/bf561-ezkit.h    |    1 +
 include/configs/blackstamp.h     |    1 +
 include/configs/cerf250.h        |    1 +
 include/configs/cm-bf533.h       |    1 +
 include/configs/cm-bf561.h       |    1 +
 include/configs/cradle.h         |    1 +
 include/configs/delta.h          |    1 +
 include/configs/dnp1110.h        |    1 +
 include/configs/gr_cpci_ax2000.h |    1 +
 include/configs/gr_ep2s60.h      |    1 +
 include/configs/innokom.h        |    1 +
 include/configs/integratorcp.h   |    1 +
 include/configs/logodl.h         |    1 +
 include/configs/lpd7a400-10.h    |    1 +
 include/configs/lpd7a404-10.h    |    1 +
 include/configs/ms7722se.h       |    1 +
 include/configs/netstar.h        |    1 +

Quoted 8 lines

 include/configs/nhk8815.h        |    1 +
 include/configs/pxa255_idp.h     |    1 +
 include/configs/versatile.h      |    1 +
 include/configs/voiceblue.h      |    1 +
 include/configs/xaeniax.h        |    1 +
 include/configs/xm250.h          |    1 +
 include/configs/xsengine.h       |    1 +
 include/configs/zylonite.h       |    1 +

This is a pretty long list of boards which is affected. How many of these have actuaaly been tested with this patch, and which ones, and to what extent?

Quoted 9 lines

drivers/net/smc91111.c int get_rom_mac (uchar *v_rom_mac) · line 1575

Quoted 6 lines

drivers/net/smc91111.c +5 -0

+#ifdef CONFIG_SMC_USE_32_BIT
+#define USE_32BIT 1
+#else
+#define USE_32BIT 0
+#endif

Above you get rid of the USE_32BIT stuff; here you re-introduce it. Why?

yes it's different here it's to declare that you want to activate the 32bit for the default driver init

Quoted 3 lines

This is a pretty long list of boards which is affected. How many of these have actuaaly been tested with this patch, and which ones, and to what extent?

none will be impact as the share the same init and I've not modify any config just active the multi support

and the code is tested on custom boards (4 differents) + nhk8815

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
 include/net.h |    2 ++
 net/eth.c     |   28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

net/eth.c +4 -0

+{
+	sprintf(enetvar, num ? "eth%daddr" : "ethaddr", num);
+}
+
 void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
 {
 	char *end;

@@ -53,6 +59,24 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr)
 
 	return setenv(name, buf);

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 5 lines · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
 include/net.h |    2 ++
 net/eth.c     |   28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

NAK.

First, there are formal issues:

- The subject line is way too long.

- There is no commit message and no description what this patch is supposed to do or to fix. Why should we add it?

include/net.h extern void eth_set_enetaddr(int num, char* a); /* Set new MAC address */ · line 128

Quoted 10 lines

index 2a8a12d..dc4ae41 100644

include/net.h +2 -0

@@ -124,6 +124,8 @@ extern void eth_set_enetaddr(int num, char* a);	/* Set new MAC address */
 extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
 extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
 extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
+extern int eth_getenv_num_enetaddr(int num, uchar *enetaddr);
+extern int eth_setenv_num_enetaddr(int num, const uchar *enetaddr);

What are these functions god for? Are they by any chance duplicationg existing code, got example eth_getenv_enetaddr_by_index() ?

Quoted 3 lines

- There is no commit message and no description what this patch is supposed to do or to fix. Why should we add it?

just to stop to duplicate this in every driver

Quoted 4 lines

Quoted 4 lines

index 2a8a12d..dc4ae41 100644

include/net.h +0 -0

@@ -124,6 +124,8 @@ extern void eth_set_enetaddr(int num, char* a);	/* Set new MAC address */

Quoted 8 lines

include/net.h extern void eth_set_enetaddr(int num, char* a); /* Set new MAC address */ · line 128

Quoted 5 lines

include/net.h +2 -0

 extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
 extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
 extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
+extern int eth_getenv_num_enetaddr(int num, uchar *enetaddr);
+extern int eth_setenv_num_enetaddr(int num, const uchar *enetaddr);

What are these functions god for? Are they by any chance duplicationg existing code, got example eth_getenv_enetaddr_by_index() ?

the get yes but not there is no set

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
 include/net.h |    1 +
 net/eth.c     |   17 +++++------------
 2 files changed, 6 insertions(+), 12 deletions(-)

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 5 lines · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
 include/net.h |    1 +
 net/eth.c     |   17 +++++------------
 2 files changed, 6 insertions(+), 12 deletions(-)

What exactly is the problem you are addressing with this patch?

Please provide a commit message that explains what is going on, and what is being changed or fixed.

Quoted 12 lines · Wolfgang Denk

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 6 lines · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
 include/net.h |    1 +
 net/eth.c     |   17 +++++------------
 2 files changed, 6 insertions(+), 12 deletions(-)

What exactly is the problem you are addressing with this patch?

Quoted 3 lines

Please provide a commit message that explains what is going on, and what is being changed or fixed.

simple it impossible to known what will be your device index in the driver specially when you have 2 or more drivers instance once or more so you can not update the mac addres in the env if you want to do it as we do on smc91111 as we can not known eth%daddr you are
which allow us the avoid to read the eeprom every time we want to use the eth

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 10 lines · Wolfgang Denk

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 5 lines · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
include/net.h | 1 +
net/eth.c | 17 +++++------------

2 files changed, 6 insertions(+), 12 deletions(-)

What exactly is the problem you are addressing with this patch?

Quoted 8 lines

Quoted 2 lines

Please provide a commit message that explains what is going on, and what is being changed or fixed.

simple it impossible to known what will be your device index in the driver specially when you have 2 or more drivers instance once or more so you can not update the mac addres in the env if you want to do it as we do on smc91111 as we can not known eth%daddr you are
which allow us the avoid to read the eeprom every time we want to use the eth

While I'm not completely opposed to the idea of tracking indices, it's simply not true that you don't know the indices of the controllers on your board. They're all instantiated in board_eth_init(), so the first will be 0 and the second will be 1 etc. If you had a mix of devices and they were found by probing as in Linux, it would be different. Here in U-boot, ordering is deterministic and dictated by the developer.

BTW - this is hardly the first driver that can have multiple
instances. Others, such as TSEC, seem to be managing just fine.

Quoted 8 lines

While I'm not completely opposed to the idea of tracking indices, it's simply not true that you don't know the indices of the controllers on your board. They're all instantiated in board_eth_init(), so the first will be 0 and the second will be 1 etc. If you had a mix of devices and they were found by probing as in Linux, it would be different. Here in U-boot, ordering is deterministic and dictated by the developer.

but you can not known it in the driver specially if you have multi different MAC chip
2 SMC91111
+ 1 SOC MAC

and you will add it in the order of preferance to use for the board to let u-boot try to connect in the order of your preference

Quoted 26 lines · Ben Warren

Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 12 lines · Wolfgang Denk

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 6 lines · In message <[email protected]> you wrote:

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

---
include/net.h | 1 +
net/eth.c | 17 +++++------------

2 files changed, 6 insertions(+), 12 deletions(-)

What exactly is the problem you are addressing with this patch?

Quoted 10 lines

Quoted 3 lines

Please provide a commit message that explains what is going on, and what is being changed or fixed.

simple it impossible to known what will be your device index in the driver specially when you have 2 or more drivers instance once or more so you can not update the mac addres in the env if you want to do it as we do on smc91111 as we can not known eth%daddr you are
which allow us the avoid to read the eeprom every time we want to use the eth

Quoted 8 lines

While I'm not completely opposed to the idea of tracking indices, it's simply not true that you don't know the indices of the controllers on your board. They're all instantiated in board_eth_init(), so the first will be 0 and the second will be 1 etc. If you had a mix of devices and they were found by probing as in Linux, it would be different. Here in U-boot, ordering is deterministic and dictated by the developer.

but you can not known it in the driver specially if you have multi different MAC chip
2 SMC91111
+ 1 SOC MAC

and you will add it in the order of preferance to use for the board to let u-boot try to connect in the order of your preference

as example this order
SOC_M_0
SMC9111_0
SMC9111_1

so you can do this

board_eth_init() {
	call cpu_eth_init
	register smc91111_0
	register smc91111_1
}

in an other board I can prefer

board_eth_init() {
	call cpu_eth_init
	register smc91111_0
	register smc91111_1
}

so instead of duplicate the info and the code you just allow the driver to known is index

Quoted 3 lines

BTW - this is hardly the first driver that can have multiple
instances. Others, such as TSEC, seem to be managing just fine.

each drivers do his own way

this one is generic

Best Regards,
J.

Quoted 5 lines

What exactly is the problem you are addressing with this patch?

Please provide a commit message that explains what is going on, and what is being changed or fixed.

simple it impossible to known what will be your device index in the driver specially when you have 2 or more drivers instance once or more

so you can not update the mac addres in the env if you want to do it as we do on smc91111 as we can not known eth%daddr you are
which allow us the avoid to read the eeprom every time we want to use the eth