Re: [U-Boot] [PATCH v2] usb: eth: asix88179: add ability to modify MAC address
Am 12.01.2015 um 17:54 schrieb Marek Vasut: On Monday, January 12, 2015 at 05:51:16 PM, Rene Griessl wrote: This patch enables U-Boot to modify the MAC address of the AX88179. Tested on RECS5250 (similar to Arndale5250) Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de --- Changes for v2: - removed memcpy Hi! what was the reason for the memcpy() in the first place please ? I copied the function from the asix.c driver. There it is needed for the cache aligned buffer. But in this case the alignment is already implemented in the write function. So you were right, it is not needed here! Otherwise, Reviewed-by: Marek Vasut ma...@denx.de Best regards, Marek Vasut -- Dipl.-Ing. René Griessl Universität Bielefeld AG Kognitronik Sensorik Exzellenzcluster Cognitive Interaction Technology (CITEC) Inspiration 1 (Zehlendorfer Damm 199) 33615 Bielefeld Telefon : +49 521-106-67362 Fax : +49 521-106-12348 eMail : rgrie...@cit-ec.uni-bielefeld.de Internet: http://www.ks.cit-ec.uni-bielefeld.de/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 2/3] usb: eth: add ASIX AX88179 DRIVER
Is there a reason you can't implement write_hwaddr() so that it will behave like other NICs (u-boot is the highest priority source of the MAC address). The AX88179 uses an external flash to store the MAC address. So for my application I do not want the user to be able to change it, since it is a unique ID inside the server cluster. I could add it as compiler option. Maybe it is even better to implement my behavior as option, since it differs from standard. I think it will be finished mid of january, so if it is OK for you in a separate patch. Looks good to me otherwise. Thanks, -Joe Thanks, Rene ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V4 2/3] usb: eth: add ASIX AX88179 DRIVER
On 08.11.2014 12:31, Marek Vasut wrote: On Friday, November 07, 2014 at 04:53:48 PM, Rene Griessl wrote: This patch adds driver support for the ASIX AX88179 USB3.0 to GbE network adapter. Driver has been tested on the RECS5250 COM module (similar to ARDALE5250). Testcase was DHCP and PXE boot. Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de Reviewed-by: Marek Vasut ma...@denx.de Joe, can you take a look? Best regards, Marek Vasut Please update status. Best regards, Rene Griessl ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
Hello Andy, today I'm back from holiday. There is a lot of different work waiting for me, so I think it will be submitted by the end of next week. Did you try the V3 Patch? br René Am 06.10.2014 19:45, schrieb Andy Pont: Hello Rene, Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER changes in v3: -added all compatible devices from linux driver -fixed issues from review changes in v2: -added usb_ether.h to change list -added 2nd patch to enable driver for arndale board Following the additional comments that came from Marek do you know when you will be submitted a v4 of this patch? Thanks for your effort on this. Regards, Andy. -- Dipl.-Ing. René Griessl Universität Bielefeld AG Kognitronik Sensorik Exzellenzcluster Cognitive Interaction Technology (CITEC) Inspiration 1 (Zehlendorfer Damm 199) 33615 Bielefeld Telefon : +49 521-106-67362 Fax : +49 521-106-12348 eMail : rgrie...@cit-ec.uni-bielefeld.de Internet: http://www.ks.cit-ec.uni-bielefeld.de/ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
);
+
+ packet_len = length;
+ cpu_to_le32s(packet_len);
+
+ memcpy(msg, packet_len, sizeof(packet_len));
+
+ tx_hdr2 = 0;
+ if (((length + 8) % 0x200 /*frame_size*/) == 0)
Define the frame size as a named constant, then use it here.
+ tx_hdr2 |= 0x80008000; /* Enable padding */
+
+ cpu_to_le32s(tx_hdr2);
+
+ memcpy(msg + sizeof(packet_len), tx_hdr2, sizeof(tx_hdr2));
+
+ memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
+ (void *)packet, length);
+
+ err = usb_bulk_msg(dev-pusb_dev,
+ usb_sndbulkpipe(dev-pusb_dev, dev-ep_out),
+ (void *)msg,
+ length + sizeof(packet_len) + sizeof(tx_hdr2),
+ actual_len,
+ USB_BULK_SEND_TIMEOUT);
+ debug(Tx: len = %u, actual = %u, err = %d\n,
+ length + sizeof(packet_len), actual_len, err);
+
+ return err;
+}
[...]
+/* Probe to see if a new device is actually an asix device */
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+ struct ueth_data *ss)
+{
+ struct usb_interface *iface;
+ struct usb_interface_descriptor *iface_desc;
+ int ep_in_found = 0, ep_out_found = 0;
+ int i;
+
+ /* let's examine the device now */
+ iface = dev-config.if_desc[ifnum];
+ iface_desc = dev-config.if_desc[ifnum].desc;
+
+ for (i = 0; asix_dongles[i].vendor != 0; i++) {
+ if (dev-descriptor.idVendor == asix_dongles[i].vendor
+ dev-descriptor.idProduct == asix_dongles[i].product)
+ /* Found a supported dongle */
+ break;
+ }
+
+ if (asix_dongles[i].vendor == 0)
+ return 0;
+
+ memset(ss, 0, sizeof(struct ueth_data));
+
+ /* At this point, we know we've got a live one */
+ debug(\n\nUSB Ethernet device detected: %#04x:%#04x\n,
+ dev-descriptor.idVendor, dev-descriptor.idProduct);
+
+ /* Initialize the ueth_data structure with some useful info */
+ ss-ifnum = ifnum;
+ ss-pusb_dev = dev;
+ ss-subclass = iface_desc-bInterfaceSubClass;
+ ss-protocol = iface_desc-bInterfaceProtocol;
+
+ /* alloc driver private */
+ ss-dev_priv = calloc(1, sizeof(struct asix_private));
+ if (!ss-dev_priv)
+ return 0;
+
+ ((struct asix_private *)ss-dev_priv)-flags = asix_dongles[i].flags;
+
+ /*
+* We are expecting a minimum of 3 endpoints - in, out (bulk), and
+* int. We will ignore any others.
+*/
+ for (i = 0; i iface_desc-bNumEndpoints; i++) {
+ /* is it an BULK endpoint? */
+ if ((iface-ep_desc[i].bmAttributes
+USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
if (! ...)
continue;
if ((ep_addr USB_DIR_IN) !ep_in_found) {
something
}
if (!(ep_addr USB_DIR_IN) !ep_out_found) {
something
}
+ u8 ep_addr = iface-ep_desc[i].bEndpointAddress;
+ if (ep_addr USB_DIR_IN) {
+ if (!ep_in_found) {
+ ss-ep_in = ep_addr
+ USB_ENDPOINT_NUMBER_MASK;
+ ep_in_found = 1;
+ }
+ } else {
+ if (!ep_out_found) {
+ ss-ep_out = ep_addr
+ USB_ENDPOINT_NUMBER_MASK;
+ ep_out_found = 1;
+ }
+ }
See above how to trim down the indent here.
[...]
--
Dipl.-Ing. René Griessl
Universität Bielefeld
AG Kognitronik Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld
Telefon : +49 521-106-67362
Fax : +49 521-106-12348
eMail : rgrie...@cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
On Friday, September 26, 2014 at 11:35:02 AM, René Griessl wrote:
changes in v3:
-added all compatible devices from linux driver
-fixed issues from review
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
The changelog goes to the [*] marker below. And you're missing a
meaningful commit message too. Also, if the driver is pulled from Linux,
please specify from which commit in there, so future developers might
re-sync the driver if needed be and they'd know from which point the
driver was derived.
Were exactly can I find the marker?
Well, in git if you pulled the driver from Linux. Use git log path/to/file(s)
With git log I can see the commit messages, right?
Does that mean, that I have omit the change-log in the commit message,
or only write the
last changes in there?
[...]
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+ int flags;
+};
This thing is a little iffy ... do you really need a full-blown struct
here or can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see
line 589 of c-file)
OK, I see assignment of the -flags , but I don't see where this is ever used.
Is this -flags used at all ?
Well thats correct. In the asix.c driver the flags are used to handle
small differences between
the devices. This is left inside for future work, if it becomes
necessary to handle things different.
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
index, + u16 size, void *data)
+{
+ int len;
+
+ debug(asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
size=%d\n, +cmd, value, index, size);
+
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
I think if you really enable the debug to generate a printf() , the
compiler will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one
has not declaration?
You have this:
int len;
printf(...); /* this comes from the debug() if debug is enabled */
char blah.; /* This comes from the expansion of ALLOC_CACHE_ALIGN_BUFFER()*/
See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
definition .
OK, now I see. Then I have a variable definition after the printf.
[...]
+ if (link_detected) {
+ if (timeout != 0)
+ printf(done.\n);
+ return 0;
Where does this return 0; belong to ?
it quits the init function, because a link is detected. Is it more
likely to put a goto here?
It's the alignment that is confusing. Do you exit only if (!timeout) is true or
do you exit unconditionally if (link_detected) ?
I changed the name of the variable to time_waited and the check is now
(time_waited 0)
so done is only printed when you really had to wait for the connection.
If the connection
was already established the messages will not be printed.
And the return has one tab less now.
[...]
--
Dipl.-Ing. René Griessl
Universität Bielefeld
AG Kognitronik Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld
Telefon : +49 521-106-67362
Fax : +49 521-106-12348
eMail : rgrie...@cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] usb: eth: add ASIX AX88179 DRIVER
Am 10.09.2014 17:00, schrieb Marek Vasut:
On Wednesday, September 10, 2014 at 12:00:29 PM, René Griessl wrote:
Am 09.09.2014 16:34, schrieb Marek Vasut:
On Wednesday, September 03, 2014 at 04:31:20 PM, Rene Griessl wrote:
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
I see that in Linux, there is asix_common.c stuff. Can this driver share
any parts with drivers/net/ax88* ?
The asix_common.c includes asix.h. There you see that the common driver
supports following devices:
AX88172
AX88772
AX88178
but it is not supporting AX88179 and AX88178a, they have a separate
driver called ax88179_178a.c
These two have a different style in accessing MAC registers and PHY
I didn't look deep enough. The 88179 driver is a completely separate driver, not
sharing any code what-so-ever with the other ASIX driver, yes ?
The USB read and write cmd functions are similar and the probe function
is the same. So they are
sharing 6% of code.
What do you have in mind? Do you want to build one driver supporting all
devices?
[...]
+ buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+ }
+ else {
+ }
Uh, this check needs some rework, right ? Also, you want to lint your
patches with ./scripts/checkpatch.pl tool before resubmitting.
was OK for ./scripts/checkpatch.pl
but I can change that
This is rather surprising, but you're right. Please fix this up, the else can be
dropped and the trailing } can be indented just under the if () clause.
OK
+ return ret;
+}
+
+
+static int asix_read_mac(struct eth_device *eth)
+{
+ struct ueth_data *dev = (struct ueth_data *)eth-priv;
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
+
+ if (dev-pusb_dev-descriptor.idProduct == 0x1790) {
+ asix_read_cmd(dev, AX_ACCESS_MAC, 0x10, 6, 6, buf);
+ debug(asix_read_mac() returning 0x%02x%02x%02x%02x%02x%02x\n,
+ buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+ memcpy(eth-enetaddr, buf, ETH_ALEN);
+ }
Same here.
+ return 0;
+}
+
+
+
+static int asix_basic_reset(struct ueth_data *dev)
+{
+ ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
Why does the buffer need to be aligned here ? It's just a buffer that is
not used for DMA, no ?
+ u16 *tmp16;
Is it because you were getting unaligned accesses , since when the buffer
itself was not aligned and you did cast it to u16, the CPU triggered
unaligned access ?
Thats right, if I do not align I get unaligned accesses during USB
communication.
Is there a smarter solution for that?
Oh I see. The smart solution would be to add bounce buffer into the USB stack,
but unless you want to dive into this, this solution would be OK here.
OK, then I will leave it this way.
--
Dipl.-Ing. René Griessl
Universität Bielefeld
AG Kognitronik Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld
Telefon : +49 521-106-67362
Fax : +49 521-106-12348
eMail : rgrie...@cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] usb: eth: add ASIX AX88179 DRIVER
Am 09.09.2014 16:34, schrieb Marek Vasut:
On Wednesday, September 03, 2014 at 04:31:20 PM, Rene Griessl wrote:
changes in v2:
-added usb_ether.h to change list
-added 2nd patch to enable driver for arndale board
Signed-off-by: Rene Griessl rgrie...@cit-ec.uni-bielefeld.de
I see that in Linux, there is asix_common.c stuff. Can this driver share any
parts with drivers/net/ax88* ?
The asix_common.c includes asix.h. There you see that the common driver
supports following devices:
AX88172
AX88772
AX88178
but it is not supporting AX88179 and AX88178a, they have a separate
driver called ax88179_178a.c
These two have a different style in accessing MAC registers and PHY
---
drivers/usb/eth/Makefile| 3 +
drivers/usb/eth/asix88179.c | 641
drivers/usb/eth/usb_ether.c |
7 +
include/usb_ether.h | 6 +
4 files changed, 657 insertions(+)
create mode 100644 drivers/usb/eth/asix88179.c
diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index 94551c4..fad4acd 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -8,5 +8,8 @@ obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
ifdef CONFIG_USB_ETHER_ASIX
obj-y += asix.o
endif
+ifdef CONFIG_USB_ETHER_ASIX_88179
+obj-y += asix88179.o
+endif
This should be obj-$(CONFIG) as seen below. Fix the asix one in a separate
patch please.
[...]
OK
+/* ASIX AX88179 based USB 3.0 Ethernet Devices */
+#define AX88179_PHY_ID 0x03
+#define AX_EEPROM_LEN 0x100
+#define AX88179_EEPROM_MAGIC 0x17900b95
+#define AX_MCAST_FLTSIZE 8
+#define AX_MAX_MCAST 64
+#define AX_INT_PPLS_LINK ((u32)BIT(16))
The u32 cast is not needed. Also, please drop the BIT() macro, it's just
obfuscating the code, just use (1 16) instead. Please fix globally.
OK (was just copy'n'paste from the linux driver)
+#define AX_RXHDR_L4_TYPE_MASK 0x1c
+#define AX_RXHDR_L4_TYPE_UDP 4
+#define AX_RXHDR_L4_TYPE_TCP 16
+#define AX_RXHDR_L3CSUM_ERR2
+#define AX_RXHDR_L4CSUM_ERR1
+#define AX_RXHDR_CRC_ERR ((u32)BIT(29))
+#define AX_RXHDR_DROP_ERR ((u32)BIT(31))
+#define AX_ACCESS_MAC 0x01
+#define AX_ACCESS_PHY 0x02
+#define AX_ACCESS_EEPROM 0x04
+#define AX_ACCESS_EFUS 0x05
+#define AX_PAUSE_WATERLVL_HIGH 0x54
+#define AX_PAUSE_WATERLVL_LOW 0x55
[...]
+static inline int asix_get_phy_addr(struct ueth_data *dev)
+{
+ ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 2);
+ int ret = -1;
+ if (dev-pusb_dev-descriptor.idProduct == 0x1790) {
+ ret = asix_read_cmd(dev, AX_ACCESS_MAC, 0x10, 6, 6, buf);
+ debug(asix_get_phy_addr() returning
0x%02x%02x%02x%02x%02x%02x\n,
+ buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+ }
+ else {
+ }
Uh, this check needs some rework, right ? Also, you want to lint your patches
with ./scripts/checkpatch.pl tool before resubmitting.
was OK for ./scripts/checkpatch.pl
but I can change that
+ return ret;
+}
+
+
+static int asix_read_mac(struct eth_device *eth)
+{
+ struct ueth_data *dev = (struct ueth_data *)eth-priv;
+ ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
+
+ if (dev-pusb_dev-descriptor.idProduct == 0x1790) {
+ asix_read_cmd(dev, AX_ACCESS_MAC, 0x10, 6, 6, buf);
+ debug(asix_read_mac() returning 0x%02x%02x%02x%02x%02x%02x\n,
+ buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+ memcpy(eth-enetaddr, buf, ETH_ALEN);
+ }
+ return 0;
+}
+
+
+
+static int asix_basic_reset(struct ueth_data *dev)
+{
+ ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
Why does the buffer need to be aligned here ? It's just a buffer that is not
used for DMA, no ?
+ u16 *tmp16;
Is it because you were getting unaligned accesses , since when the buffer itself
was not aligned and you did cast it to u16, the CPU triggered unaligned access ?
Thats right, if I do not align I get unaligned accesses during USB
communication.
Is there a smarter solution for that?
+ u8 *tmp;
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
