Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
Dear Joe Hershberger, Hi Marek, On Fri, Dec 16, 2011 at 2:36 PM, Marek Vasut marek.va...@gmail.com wrote: On Friday 16 December 2011 13:13:33 Marek Vasut wrote: On Friday 16 December 2011 12:33:53 Mike Frysinger wrote: rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to CONFIG_NE2000_IO_STRIDE, and move them both to the board config header. then you get one unified set: #define DP_IN(_b_, _o_, _d_) \ (_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \ CONFIG_NE2000_IO_OFFSET)); etc... if you really wanted to clean up the driver, the DP_XXX funcs would get turned into C code as static inline helpers, and the base + register offset would get turned into a C struct. Ok, so if you had two different piece of hardware that had different NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ? do you actually have this issue ? there are plenty of theoretical situations like this which would break a significant number (majority?) of drivers in the tree. so unless this is a real case, i'd ignore it for now and stick with what optimizes away to no overhead. Sadly, I almost do. Not now of course, but eventually, I'll be there :-( Are you planning to improve this patch? No, I'll eventually rework this one alongside the whole DM crap. I think at the very least you should have a compile option to enable this functional interface presumably behind a macro, but every user of NE2000 should not have to pay the price. However, like Mike suggested, you should only add this complexity if you actually have this problem on a board. Thanks, -Joe Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
Hi Marek, On Fri, Dec 16, 2011 at 2:36 PM, Marek Vasut marek.va...@gmail.com wrote: On Friday 16 December 2011 13:13:33 Marek Vasut wrote: On Friday 16 December 2011 12:33:53 Mike Frysinger wrote: rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to CONFIG_NE2000_IO_STRIDE, and move them both to the board config header. then you get one unified set: #define DP_IN(_b_, _o_, _d_) \ (_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \ CONFIG_NE2000_IO_OFFSET)); etc... if you really wanted to clean up the driver, the DP_XXX funcs would get turned into C code as static inline helpers, and the base + register offset would get turned into a C struct. Ok, so if you had two different piece of hardware that had different NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ? do you actually have this issue ? there are plenty of theoretical situations like this which would break a significant number (majority?) of drivers in the tree. so unless this is a real case, i'd ignore it for now and stick with what optimizes away to no overhead. Sadly, I almost do. Not now of course, but eventually, I'll be there :-( Are you planning to improve this patch? I think at the very least you should have a compile option to enable this functional interface presumably behind a macro, but every user of NE2000 should not have to pay the price. However, like Mike suggested, you should only add this complexity if you actually have this problem on a board. Thanks, -Joe ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Monday 12 December 2011 05:17:49 Marek Vasut wrote:
On Sunday 11 December 2011 08:42:07 Marek Vasut wrote:
On Saturday 10 December 2011 20:09:30 Marek Vasut wrote:
Introduce ne2k_register_io(in, out), which allows user to supply
two functions. One for reading data from the card, the other for
writing data to the card. Then introduce drivers' private data,
which carry pointers to these functions and are passed throughout
the driver.
where are the users of this new API ? as it stands, i just see
bloat. every register access is now an indirect function call ?
what's the point
Go to ... drivers/net/ax88796.h ... and check how it's done now. It's
just wrong. Now for .03 release I have pxa3xx support ready which uses
just this chip and adding more sh^Htuff to that fill would be even
worse bloat.
i agree, that code is terrible. however, those code paths can be
trivially merged without the proposed bloat yours brings in.
So what's your suggested awesome clean solution?
there's no need to get snarky
rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to
CONFIG_NE2000_IO_STRIDE, and move them both to the board config header.
then you get one unified set:
#define DP_IN(_b_, _o_, _d_) \
(_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \
CONFIG_NE2000_IO_OFFSET));
etc...
if you really wanted to clean up the driver, the DP_XXX funcs would get turned
into C code as static inline helpers, and the base + register offset would get
turned into a C struct.
further, that code base
isn't even used by the ne2000 driver.
What are you talking about, did you even bother to look?
might want to cut the attitude. it's really not adding anything.
of course i looked and i saw ne2000.h defining DP_OUT/etc... but i missed the
ugly ifdef logic with CONFIG_DRIVER_AX88796L in ne2000_base.c.
so again, the question stands: what exactly do you need to do different ?
looks to me like the DP_* macros should get punted in favor of io.h
accessors, and the register offsets rewritten into C structs.
Sure, but not every hardware accesses the registers the same way.
which is why we have asm/io.h in the first place.
on a semi-related note, these vu_{short,char,etc...} types need to get culled
from the code base. they're volatile markings in disguise ...
-mike
signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Monday 12 December 2011 05:17:49 Marek Vasut wrote:
On Sunday 11 December 2011 08:42:07 Marek Vasut wrote:
On Saturday 10 December 2011 20:09:30 Marek Vasut wrote:
Introduce ne2k_register_io(in, out), which allows user to supply
two functions. One for reading data from the card, the other for
writing data to the card. Then introduce drivers' private data,
which carry pointers to these functions and are passed throughout
the driver.
where are the users of this new API ? as it stands, i just see
bloat. every register access is now an indirect function call ?
what's the point
Go to ... drivers/net/ax88796.h ... and check how it's done now. It's
just wrong. Now for .03 release I have pxa3xx support ready which
uses just this chip and adding more sh^Htuff to that fill would be
even worse bloat.
i agree, that code is terrible. however, those code paths can be
trivially merged without the proposed bloat yours brings in.
So what's your suggested awesome clean solution?
there's no need to get snarky
Well sometimes it's hard to digest the style of your comments ;-)
rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to
CONFIG_NE2000_IO_STRIDE, and move them both to the board config header.
then you get one unified set:
#define DP_IN(_b_, _o_, _d_) \
(_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \
CONFIG_NE2000_IO_OFFSET));
etc...
if you really wanted to clean up the driver, the DP_XXX funcs would get
turned into C code as static inline helpers, and the base + register
offset would get turned into a C struct.
Ok, so if you had two different piece of hardware that had different
NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ?
further, that code base
isn't even used by the ne2000 driver.
What are you talking about, did you even bother to look?
might want to cut the attitude. it's really not adding anything.
Agreed
of course i looked and i saw ne2000.h defining DP_OUT/etc... but i missed
the ugly ifdef logic with CONFIG_DRIVER_AX88796L in ne2000_base.c.
That needs handling in a separate patch.
so again, the question stands: what exactly do you need to do different
? looks to me like the DP_* macros should get punted in favor of io.h
accessors, and the register offsets rewritten into C structs.
Sure, but not every hardware accesses the registers the same way.
which is why we have asm/io.h in the first place.
on a semi-related note, these vu_{short,char,etc...} types need to get
culled from the code base. they're volatile markings in disguise ...
Agreed
M
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Friday 16 December 2011 13:13:33 Marek Vasut wrote: On Friday 16 December 2011 12:33:53 Mike Frysinger wrote: rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to CONFIG_NE2000_IO_STRIDE, and move them both to the board config header. then you get one unified set: #define DP_IN(_b_, _o_, _d_) \ (_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \ CONFIG_NE2000_IO_OFFSET)); etc... if you really wanted to clean up the driver, the DP_XXX funcs would get turned into C code as static inline helpers, and the base + register offset would get turned into a C struct. Ok, so if you had two different piece of hardware that had different NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ? do you actually have this issue ? there are plenty of theoretical situations like this which would break a significant number (majority?) of drivers in the tree. so unless this is a real case, i'd ignore it for now and stick with what optimizes away to no overhead. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Friday 16 December 2011 13:13:33 Marek Vasut wrote: On Friday 16 December 2011 12:33:53 Mike Frysinger wrote: rename ISA_OFFSET to CONFIG_NE2000_IO_OFFSET, then move the 2 to CONFIG_NE2000_IO_STRIDE, and move them both to the board config header. then you get one unified set: #define DP_IN(_b_, _o_, _d_) \ (_d_) = readw((void *)((_b_) + ((_o_) * CONFIG_NE2000_IO_STRIDE) + \ CONFIG_NE2000_IO_OFFSET)); etc... if you really wanted to clean up the driver, the DP_XXX funcs would get turned into C code as static inline helpers, and the base + register offset would get turned into a C struct. Ok, so if you had two different piece of hardware that had different NE2000_IO_OFFSET and STRIDE, running the same u-boot, how'd you handle it ? do you actually have this issue ? there are plenty of theoretical situations like this which would break a significant number (majority?) of drivers in the tree. so unless this is a real case, i'd ignore it for now and stick with what optimizes away to no overhead. Sadly, I almost do. Not now of course, but eventually, I'll be there :-( M ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Sunday 11 December 2011 08:42:07 Marek Vasut wrote: On Saturday 10 December 2011 20:09:30 Marek Vasut wrote: Introduce ne2k_register_io(in, out), which allows user to supply two functions. One for reading data from the card, the other for writing data to the card. Then introduce drivers' private data, which carry pointers to these functions and are passed throughout the driver. where are the users of this new API ? as it stands, i just see bloat. every register access is now an indirect function call ? what's the point Go to ... drivers/net/ax88796.h ... and check how it's done now. It's just wrong. Now for .03 release I have pxa3xx support ready which uses just this chip and adding more sh^Htuff to that fill would be even worse bloat. i agree, that code is terrible. however, those code paths can be trivially merged without the proposed bloat yours brings in. So what's your suggested awesome clean solution? further, that code base isn't even used by the ne2000 driver. What are you talking about, did you even bother to look? so again, the question stands: what exactly do you need to do different ? looks to me like the DP_* macros should get punted in favor of io.h accessors, and the register offsets rewritten into C structs. Sure, but not every hardware accesses the registers the same way. So I'm open for your suggestions on how to do it properly, apparently you have much better idea up your sleeve. M ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Saturday 10 December 2011 20:09:30 Marek Vasut wrote: Introduce ne2k_register_io(in, out), which allows user to supply two functions. One for reading data from the card, the other for writing data to the card. Then introduce drivers' private data, which carry pointers to these functions and are passed throughout the driver. where are the users of this new API ? as it stands, i just see bloat. every register access is now an indirect function call ? what's the point Go to ... drivers/net/ax88796.h ... and check how it's done now. It's just wrong. Now for .03 release I have pxa3xx support ready which uses just this chip and adding more sh^Htuff to that fill would be even worse bloat. M ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Sunday 11 December 2011 08:42:07 Marek Vasut wrote: On Saturday 10 December 2011 20:09:30 Marek Vasut wrote: Introduce ne2k_register_io(in, out), which allows user to supply two functions. One for reading data from the card, the other for writing data to the card. Then introduce drivers' private data, which carry pointers to these functions and are passed throughout the driver. where are the users of this new API ? as it stands, i just see bloat. every register access is now an indirect function call ? what's the point Go to ... drivers/net/ax88796.h ... and check how it's done now. It's just wrong. Now for .03 release I have pxa3xx support ready which uses just this chip and adding more sh^Htuff to that fill would be even worse bloat. i agree, that code is terrible. however, those code paths can be trivially merged without the proposed bloat yours brings in. further, that code base isn't even used by the ne2000 driver. so again, the question stands: what exactly do you need to do different ? looks to me like the DP_* macros should get punted in favor of io.h accessors, and the register offsets rewritten into C structs. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
Introduce ne2k_register_io(in, out), which allows user to supply two functions.
One for reading data from the card, the other for writing data to the card. Then
introduce drivers' private data, which carry pointers to these functions and are
passed throughout the driver.
The private data will carry more entries later.
Signed-off-by: Marek Vasut marek.va...@gmail.com
---
drivers/net/ne2000_base.c | 318 +++--
include/netdev.h |2 +
2 files changed, 191 insertions(+), 129 deletions(-)
NOTE: Formating not cleaned up, only relevant change made!
diff --git a/drivers/net/ne2000_base.c b/drivers/net/ne2000_base.c
index 88f2b37..35a8581 100644
--- a/drivers/net/ne2000_base.c
+++ b/drivers/net/ne2000_base.c
@@ -76,10 +76,7 @@ Add SNMP
#include command.h
#include net.h
#include malloc.h
-
-/* forward definition of function used for the uboot interface */
-void uboot_push_packet_len(int len);
-void uboot_push_tx_done(int key, int val);
+#include errno.h
/* NE2000 base header file */
#include ne2000_base.h
@@ -94,12 +91,37 @@ void uboot_push_tx_done(int key, int val);
static dp83902a_priv_data_t nic; /* just one instance of the card supported */
+uint32_t ne2k_default_in(uint8_t *addr, uint32_t offset)
+{
+ uint32_t ret;
+ DP_IN(addr, offset, ret);
+ return ret;
+}
+
+void ne2k_default_out(uint8_t *addr, uint32_t offset, uint32_t val)
+{
+ DP_OUT(addr, offset, val);
+}
+
+struct ne2k_io_accessors {
+ uint32_t(*in)(uint8_t *addr, uint32_t offset);
+ void(*out)(uint8_t *addr, uint32_t offset, uint32_t val);
+};
+
+struct ne2k_private_data {
+ struct ne2k_io_accessorsio;
+};
+
+/* forward definition of function used for the uboot interface */
+void uboot_push_packet_len(struct ne2k_private_data *pdata, int len);
+void uboot_push_tx_done(int key, int val);
+
/**
* This function reads the MAC address from the serial EEPROM,
* used if PROM read fails. Does nothing for ax88796 chips (sh boards)
*/
static bool
-dp83902a_init(unsigned char *enetaddr)
+dp83902a_init(struct eth_device *dev)
{
dp83902a_priv_data_t *dp = nic;
u8* base;
@@ -116,13 +138,17 @@ dp83902a_init(unsigned char *enetaddr)
DEBUG_LINE();
#if defined(NE2000_BASIC_INIT)
+ unsigned char *enetaddr = dev-enetaddr;
+ struct ne2k_private_data *pdata = dev-priv;
+ const struct ne2k_io_accessors *io = pdata-io;
+
/* AX88796L doesn't need */
/* Prepare ESA */
- DP_OUT(base, DP_CR, DP_CR_NODMA | DP_CR_PAGE1); /* Select page 1 */
+ io-out(base, DP_CR, DP_CR_NODMA | DP_CR_PAGE1);/* Select page
1 */
/* Use the address from the serial EEPROM */
for (i = 0; i 6; i++)
- DP_IN(base, DP_P1_PAR0+i, dp-esa[i]);
- DP_OUT(base, DP_CR, DP_CR_NODMA | DP_CR_PAGE0); /* Select page 0 */
+ dp-esa[i] = io-in(base, DP_P1_PAR0+i);
+ io-out(base, DP_CR, DP_CR_NODMA | DP_CR_PAGE0);/* Select page
0 */
printf(NE2000 - %s ESA: %02x:%02x:%02x:%02x:%02x:%02x\n,
eeprom,
@@ -139,16 +165,18 @@ dp83902a_init(unsigned char *enetaddr)
}
static void
-dp83902a_stop(void)
+dp83902a_stop(struct eth_device *dev)
{
dp83902a_priv_data_t *dp = nic;
+ struct ne2k_private_data *pdata = dev-priv;
+ const struct ne2k_io_accessors *io = pdata-io;
u8 *base = dp-base;
DEBUG_FUNCTION();
- DP_OUT(base, DP_CR, DP_CR_PAGE0 | DP_CR_NODMA | DP_CR_STOP);/*
Brutal */
- DP_OUT(base, DP_ISR, 0xFF); /* Clear any pending interrupts
*/
- DP_OUT(base, DP_IMR, 0x00); /* Disable all interrupts */
+ io-out(base, DP_CR, DP_CR_PAGE0 | DP_CR_NODMA | DP_CR_STOP); /*
Brutal */
+ io-out(base, DP_ISR, 0xFF);/* Clear any pending interrupts
*/
+ io-out(base, DP_IMR, 0x00);/* Disable all interrupts */
dp-running = false;
}
@@ -160,9 +188,12 @@ dp83902a_stop(void)
* the hardware ready to send/receive packets.
*/
static void
-dp83902a_start(u8 * enaddr)
+dp83902a_start(struct eth_device *dev)
{
dp83902a_priv_data_t *dp = nic;
+ unsigned char *enaddr = dev-enetaddr;
+ struct ne2k_private_data *pdata = dev-priv;
+ const struct ne2k_io_accessors *io = pdata-io;
u8 *base = dp-base;
int i;
@@ -170,37 +201,37 @@ dp83902a_start(u8 * enaddr)
DEBUG_FUNCTION();
- DP_OUT(base, DP_CR, DP_CR_PAGE0 | DP_CR_NODMA | DP_CR_STOP); /* Brutal
*/
- DP_OUT(base, DP_DCR, DP_DCR_INIT);
- DP_OUT(base, DP_RBCH, 0); /* Remote byte count */
- DP_OUT(base, DP_RBCL, 0);
- DP_OUT(base, DP_RCR, DP_RCR_MON); /* Accept no packets */
- DP_OUT(base, DP_TCR, DP_TCR_LOCAL); /* Transmitter [virtually] off
*/
- DP_OUT(base, DP_TPSR, dp-tx_buf1); /* Transmitter start page */
+
Re: [U-Boot] [PATCH 1/2] NET: NE2000: Cleanup IO accessors
On Saturday 10 December 2011 20:09:30 Marek Vasut wrote: Introduce ne2k_register_io(in, out), which allows user to supply two functions. One for reading data from the card, the other for writing data to the card. Then introduce drivers' private data, which carry pointers to these functions and are passed throughout the driver. where are the users of this new API ? as it stands, i just see bloat. every register access is now an indirect function call ? what's the point ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
