Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Tuesday, August 30, 2011 08:05:19 Michal Simek wrote:
 +static void setup_mac(struct eth_device *dev)
 +{
 +/* Set the MAC address */
 +int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
 +(dev-enetaddr[1]  8) | (dev-enetaddr[0]));
 +aximac_out32(dev-iobase, XAE_UAW0_OFFSET, val);
 +
 +val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
 +val |= aximac_in32(dev-iobase, XAE_UAW1_OFFSET) 
 +~XAE_UAW1_UNICASTADDR_MASK;
 +aximac_out32(dev-iobase, XAE_UAW1_OFFSET, val);
 +}
 +
 +static int axiemac_init(struct eth_device *dev, bd_t * bis)
 +{
 +setup_mac(dev);
 
 pretty sure this should be dev-write_hwaddr

I expected it. The point is where dev-write_hwaddr is called.

I mean sequence is:
dev-write_hwaddr
u-boot network command
dev-init

If init reset device include setup mac then this sequence can't work.
But not case for my MAC - I can use it.

 
 +int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int
 dma_addr)
 
 you got base_addr right, but forgot to change dma_addr to unsigned long too ;)

Fixed.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Marek Vasut wrote:
 On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
 Marek Vasut wrote:
 On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
 Add the first axi_ethernet driver for little-endian Microblaze.

 Signed-off-by: Michal Simek mon...@monstr.eu
 ---

  drivers/net/Makefile  |1 +
  drivers/net/xilinx_axi_emac.c |  622

 + include/netdev.h 
 |

   1 +
  
  3 files changed, 624 insertions(+), 0 deletions(-)
  create mode 100644 drivers/net/xilinx_axi_emac.c

 diff --git a/drivers/net/Makefile b/drivers/net/Makefile
 index 4541eaf..ae9d4cb 100644
 --- a/drivers/net/Makefile
 +++ b/drivers/net/Makefile
 @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o

  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
  COBJS-$(CONFIG_ULI526X) += uli526x.o
  COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o

 +COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o

  COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
  COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o

 diff --git a/drivers/net/xilinx_axi_emac.c
 b/drivers/net/xilinx_axi_emac.c new file mode 100644
 index 000..ce79b80
 --- /dev/null
 +++ b/drivers/net/xilinx_axi_emac.c
 @@ -0,0 +1,622 @@
 +/*
 + * Copyright (C) 2011 Michal Simek mon...@monstr.eu
 + * Copyright (C) 2011 PetaLogix
 + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include config.h
 +#include common.h
 +#include net.h
 +#include malloc.h
 +#include asm/io.h
 +#include phy.h
 +#include miiphy.h
 +
 +/* Axi Ethernet registers offset */
 +#define XAE_IS_OFFSET 0x000C /* Interrupt status */
 +#define XAE_IE_OFFSET 0x0014 /* Interrupt enable */
 +#define XAE_RCW1_OFFSET   0x0404 /* Rx Configuration Word 1 */
 +#define XAE_TC_OFFSET 0x0408 /* Tx Configuration */
 +#define XAE_EMMC_OFFSET   0x0410 /* EMAC mode configuration */
 +#define XAE_MDIO_MC_OFFSET0x0500 /* MII Management Config */
 +#define XAE_MDIO_MCR_OFFSET   0x0504 /* MII Management Control */
 +#define XAE_MDIO_MWD_OFFSET   0x0508 /* MII Management Write Data 
 */
 +#define XAE_MDIO_MRD_OFFSET   0x050C /* MII Management Read Data 
 */
 Please use struct xae_regs {...} as the rest of the u-boot.
 struct is not useful here because it will be big with a lot of reserved
 values.
 
 I see like 10 registers here, you can use uint32_t reserved[n];

+ UAW0 and UAW1 with 0x700 offset.

struct axi_ethernet {
u32 reserved[3];
u32 is; /* 0xC: Interrupt status */
u32 reserved2;
u32 ie; /* 0x14: Interrupt enable */
u32 reserved3[251];
u32 rcw1; /* 0x404: Rx Configuration Word 1 */
u32 tc; /* 0x408: Tx Configuration */
u32 reserved4;
u32 emmc; /* 0x410: EMAC mode configuration */
u32 reserved5[59];
u32 mdio_mc; /* 0x500: MII Management Config */
u32 mdio_mcr; /* 0x504: MII Management Control */
u32 mdio_mwd; /* 0x508: MII Management Write Data */
u32 mdio_mrd; /* 0x50C: MII Management Read Data */
u32 reserved6[124];
u32 uaw0; /* 0x700: Unicast address word 0 */
u32 uaw1; /* 0x704: Unicast address word 1 */
};

Are you really sure that this is nice?


 
 +
 +/* Link setup */
 +#define XAE_EMMC_LINKSPEED_MASK   0xC000 /* Link speed */
 +#define XAE_EMMC_LINKSPD_10   0x /* Link Speed mask for 10 
 Mbit
 */ +#define XAE_EMMC_LINKSPD_100   0x4000 /* Link Speed mask for 100
 Mbit */ +#define XAE_EMMC_LINKSPD_1000 0x8000 /* Link Speed mask
 for 1000 Mbit */ +
 Use (1  n) ?
 just another solution - I prefer to use 32bit value - easier when you
 decode it by md.
 
 It's hard to read, really.

At offset 40b40410. It is really hard to decode it if values are (1  n).
With defined macro you directly see that this is on 100Mbit/s LAN.

U-Boot-mONStR md 40b40400
40b40400: ddccbbaa 6800ffee 4a00 6000...h...J...`
40b40410: 4000  000ce000 000ce000...@
40b40420: 000ce000 000ce000 000ce000 000ce000
40b40430: 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Marek Vasut]

On Thursday, September 01, 2011 09:17:35 AM Michal Simek wrote:
 Marek Vasut wrote:
  On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
  Marek Vasut wrote:
  On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
  Add the first axi_ethernet driver for little-endian Microblaze.
  
  Signed-off-by: Michal Simek mon...@monstr.eu
  ---
  

[...]

  +
  +/* Axi Ethernet registers offset */
  +#define XAE_IS_OFFSET   0x000C /* Interrupt status */
  +#define XAE_IE_OFFSET   0x0014 /* Interrupt enable */
  +#define XAE_RCW1_OFFSET 0x0404 /* Rx Configuration Word 1 */
  +#define XAE_TC_OFFSET   0x0408 /* Tx Configuration */
  +#define XAE_EMMC_OFFSET 0x0410 /* EMAC mode configuration */
  +#define XAE_MDIO_MC_OFFSET  0x0500 /* MII Management Config */
  +#define XAE_MDIO_MCR_OFFSET 0x0504 /* MII Management Control */
  +#define XAE_MDIO_MWD_OFFSET 0x0508 /* MII Management Write Data
  */ +#define XAE_MDIO_MRD_OFFSET  0x050C /* MII Management Read
  Data */
  
  Please use struct xae_regs {...} as the rest of the u-boot.
  
  struct is not useful here because it will be big with a lot of reserved
  values.
  
  I see like 10 registers here, you can use uint32_t reserved[n];
 
 + UAW0 and UAW1 with 0x700 offset.
 
 struct axi_ethernet {
   u32 reserved[3];
   u32 is; /* 0xC: Interrupt status */
   u32 reserved2;
   u32 ie; /* 0x14: Interrupt enable */
   u32 reserved3[251];
   u32 rcw1; /* 0x404: Rx Configuration Word 1 */
   u32 tc; /* 0x408: Tx Configuration */
   u32 reserved4;
   u32 emmc; /* 0x410: EMAC mode configuration */
   u32 reserved5[59];
   u32 mdio_mc; /* 0x500: MII Management Config */
   u32 mdio_mcr; /* 0x504: MII Management Control */
   u32 mdio_mwd; /* 0x508: MII Management Write Data */
   u32 mdio_mrd; /* 0x50C: MII Management Read Data */
   u32 reserved6[124];
   u32 uaw0; /* 0x700: Unicast address word 0 */
   u32 uaw1; /* 0x704: Unicast address word 1 */
 };
 
 Are you really sure that this is nice?
 

Yep, definitelly great.

  +
  +/* Link setup */
  +#define XAE_EMMC_LINKSPEED_MASK 0xC000 /* Link speed */
  +#define XAE_EMMC_LINKSPD_10 0x /* Link Speed mask for 10 
Mbit
  */ +#define XAE_EMMC_LINKSPD_100 0x4000 /* Link Speed mask for 100
  Mbit */ +#define XAE_EMMC_LINKSPD_1000   0x8000 /* Link Speed mask
  for 1000 Mbit */ +
  
  Use (1  n) ?
  
  just another solution - I prefer to use 32bit value - easier when you
  decode it by md.
  
  It's hard to read, really.
 
 At offset 40b40410. It is really hard to decode it if values are (1  n).
 With defined macro you directly see that this is on 100Mbit/s LAN.
 
 U-Boot-mONStR md 40b40400
 40b40400: ddccbbaa 6800ffee 4a00 6000...h...J...`
 40b40410: 4000  000ce000 000ce000...@
 40b40420: 000ce000 000ce000 000ce000 000ce000
 40b40430: 000ce000 000ce000 000ce000 000ce000
 
 I am fine to use 1  n solution but definitely in our repo I will use in
 way I like.

Well I see it both ways ... 0x4000 == 1  30 ... it's the same thing. On 
the other note, it's hard to count the zeroes in there AND you can mistake 0 
and 
8 in a huge series of those.

Also, you can have whatever you want in your repo if you seriously care to 
invest the energy into maintaining it just because you need to be stubborn. But 
it'd really be great if you invested that energy in a more productive manner ;-)

 
  +/* Interrupt Status/Enable/Mask Registers bit definitions */
  +#define XAE_INT_RXRJECT_MASK0x0008 /* Rx frame rejected */
  +#define XAE_INT_MGTRDY_MASK 0x0080 /* MGT clock Lock */
  +
  
  [...]
  
  same here..

Yep, 1  3 and 1  7.

  
  +#define DMAALIGN128
  +
  +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
  
  Don't use cammelcase, all lowcase please.
  
  Agree - will be done in v2.
  
Also, can't you allocate this with
  
  memalign and hide it in axidma_priv or something ?
  
  There are two things in one.
  1. Adding it to axidma_priv means that I will need to dynamically
  allocate big private structure which is bad thing to do for several eth
  devices. This is FPGA you can create a lot of MACs that's why I think
  it is better not to do so.
  2. Current style is sharing this rxframe buffer among all controllers
  because only one MAC works. Others are stopped which means that no
  packet come to them.
  
  Ok, I don't think I understand pt. 1. -- you need to keep the state for
  each of the ethernet devices on the FPGA separate, don't you.
 
 Just the whole private structure with addresses, + phy.
 

Ok, now after reading your explanation for pt.2 below, I'm starting to follow.

 
   As for pt. 2. --
 
  currently, so there's possibility, in future this won't hold?
 
 BTW: I am also sharing rx/tx buffer descriptors for 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

 +
 +/* Axi Ethernet registers offset */
 +#define XAE_IS_OFFSET   0x000C /* Interrupt status */
 +#define XAE_IE_OFFSET   0x0014 /* Interrupt enable */
 +#define XAE_RCW1_OFFSET 0x0404 /* Rx Configuration Word 1 */
 +#define XAE_TC_OFFSET   0x0408 /* Tx Configuration */
 +#define XAE_EMMC_OFFSET 0x0410 /* EMAC mode configuration */
 +#define XAE_MDIO_MC_OFFSET  0x0500 /* MII Management Config */
 +#define XAE_MDIO_MCR_OFFSET 0x0504 /* MII Management Control */
 +#define XAE_MDIO_MWD_OFFSET 0x0508 /* MII Management Write Data
 */ +#define XAE_MDIO_MRD_OFFSET  0x050C /* MII Management Read
 Data */
 Please use struct xae_regs {...} as the rest of the u-boot.
 struct is not useful here because it will be big with a lot of reserved
 values.
 I see like 10 registers here, you can use uint32_t reserved[n];
 + UAW0 and UAW1 with 0x700 offset.

 struct axi_ethernet {
  u32 reserved[3];
  u32 is; /* 0xC: Interrupt status */
  u32 reserved2;
  u32 ie; /* 0x14: Interrupt enable */
  u32 reserved3[251];
  u32 rcw1; /* 0x404: Rx Configuration Word 1 */
  u32 tc; /* 0x408: Tx Configuration */
  u32 reserved4;
  u32 emmc; /* 0x410: EMAC mode configuration */
  u32 reserved5[59];
  u32 mdio_mc; /* 0x500: MII Management Config */
  u32 mdio_mcr; /* 0x504: MII Management Control */
  u32 mdio_mwd; /* 0x508: MII Management Write Data */
  u32 mdio_mrd; /* 0x50C: MII Management Read Data */
  u32 reserved6[124];
  u32 uaw0; /* 0x700: Unicast address word 0 */
  u32 uaw1; /* 0x704: Unicast address word 1 */
 };

 Are you really sure that this is nice?

 
 Yep, definitelly great.

ok.

 
 +
 +/* Link setup */
 +#define XAE_EMMC_LINKSPEED_MASK 0xC000 /* Link speed */
 +#define XAE_EMMC_LINKSPD_10 0x /* Link Speed mask for 10 
 Mbit
 */ +#define XAE_EMMC_LINKSPD_100 0x4000 /* Link Speed mask for 100
 Mbit */ +#define XAE_EMMC_LINKSPD_1000   0x8000 /* Link Speed mask
 for 1000 Mbit */ +
 Use (1  n) ?
 just another solution - I prefer to use 32bit value - easier when you
 decode it by md.
 It's hard to read, really.
 At offset 40b40410. It is really hard to decode it if values are (1  n).
 With defined macro you directly see that this is on 100Mbit/s LAN.

 U-Boot-mONStR md 40b40400
 40b40400: ddccbbaa 6800ffee 4a00 6000...h...J...`
 40b40410: 4000  000ce000 000ce000...@
 40b40420: 000ce000 000ce000 000ce000 000ce000
 40b40430: 000ce000 000ce000 000ce000 000ce000

 I am fine to use 1  n solution but definitely in our repo I will use in
 way I like.
 
 Well I see it both ways ... 0x4000 == 1  30 ... it's the same thing. On 
 the other note, it's hard to count the zeroes in there AND you can mistake 0 
 and 
 8 in a huge series of those.
 
 Also, you can have whatever you want in your repo if you seriously care to 
 invest the energy into maintaining it just because you need to be stubborn. 
 But 
 it'd really be great if you invested that energy in a more productive manner 
 ;-)

There are two points of view. And both have con  pro.
I don't want to argue. Net custodian should decided if is OK or not.

Look at tsec.h and probably others.


 
 +/* Interrupt Status/Enable/Mask Registers bit definitions */
 +#define XAE_INT_RXRJECT_MASK0x0008 /* Rx frame rejected */
 +#define XAE_INT_MGTRDY_MASK 0x0080 /* MGT clock Lock */
 +
 [...]
 same here..
 
 Yep, 1  3 and 1  7.
 
 +#define DMAALIGN128
 +
 +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
 Don't use cammelcase, all lowcase please.
 Agree - will be done in v2.

   Also, can't you allocate this with
 memalign and hide it in axidma_priv or something ?
 There are two things in one.
 1. Adding it to axidma_priv means that I will need to dynamically
 allocate big private structure which is bad thing to do for several eth
 devices. This is FPGA you can create a lot of MACs that's why I think
 it is better not to do so.
 2. Current style is sharing this rxframe buffer among all controllers
 because only one MAC works. Others are stopped which means that no
 packet come to them.
 Ok, I don't think I understand pt. 1. -- you need to keep the state for
 each of the ethernet devices on the FPGA separate, don't you.
 Just the whole private structure with addresses, + phy.

 
 Ok, now after reading your explanation for pt.2 below, I'm starting to follow.
 
   As for pt. 2. --

 currently, so there's possibility, in future this won't hold?
 BTW: I am also sharing rx/tx buffer descriptors for dma.

 When do you expect that u-boot will be able to use several MACs in one
 time?
 
 It's not a matter of when, but -- write a correct code, it's much less burden 
 to 
 fix it later.

Agree in general.
It is always question of when. You can always do it in better way. The question 
is if
someone 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Marek Vasut]

On Thursday, September 01, 2011 10:55:31 AM Michal Simek wrote:

[...]

  I am fine to use 1  n solution but definitely in our repo I will use
  in way I like.
  
  Well I see it both ways ... 0x4000 == 1  30 ... it's the same
  thing. On the other note, it's hard to count the zeroes in there AND you
  can mistake 0 and 8 in a huge series of those.
  
  Also, you can have whatever you want in your repo if you seriously care
  to invest the energy into maintaining it just because you need to be
  stubborn. But it'd really be great if you invested that energy in a more
  productive manner ;-)
 
 There are two points of view. And both have con  pro.
 I don't want to argue. Net custodian should decided if is OK or not.
 
 Look at tsec.h and probably others.

That something's in mainline doesn't mean it's obviously correct !

[...]

  
  currently, so there's possibility, in future this won't hold?
  
  BTW: I am also sharing rx/tx buffer descriptors for dma.
  
  When do you expect that u-boot will be able to use several MACs in one
  time?
  
  It's not a matter of when, but -- write a correct code, it's much less
  burden to fix it later.
 
 Agree in general.
 It is always question of when. You can always do it in better way. The
 question is if someone will pay you for doing it in better way.

And if the code isn't accepted, they won't pay you ;-)

 If this
 feature is not important for us, make no sense to invest our time/money to
 it.

I guess having the code mainline is important ?

 
  +  /* Write new speed setting out to Axi Ethernet */
  +  aximac_out32(dev-iobase, XAE_EMMC_OFFSET, emmc_reg);
  
  Use clrsetbits() here.
  
  Not defined for Microblaze - just for ARM/PPC.
  Not going to use it.
  
  Please fix then. You're the microblaze maintainer, right?
  
  Custodian.
  
  Oh come on ...
  
  But I won't do that.
  
  I think you should.
  
  If you think that all archs should have it then move it to generic
  location which clean code duplication and I will include it.
  
  That's not the point, it's platform specific.
 
 Just compare ppc/arm implementation and they are the same. It is even pure
 generic code. Adding it to microblaze is just code duplication which is
 also not good way to go. Then in future someone will move it to generic
 location.
 There were a lot of examples in linux kernel and includes.

Ok, if it's a generic code, please submit a patch putting it into a generic 
place and the fix this driver to use it.

 
 + network driver should be platform independent which is exactly how it is
 written. Writing in the pure C should be fine.

Well there's the issue with register access and endianity. Even Linux has 
trouble with that.

Cheers

 
 Thanks,
 Michal
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Marek Vasut wrote:
 On Thursday, September 01, 2011 10:55:31 AM Michal Simek wrote:
 
 [...]
 
 I am fine to use 1  n solution but definitely in our repo I will use
 in way I like.
 Well I see it both ways ... 0x4000 == 1  30 ... it's the same
 thing. On the other note, it's hard to count the zeroes in there AND you
 can mistake 0 and 8 in a huge series of those.

 Also, you can have whatever you want in your repo if you seriously care
 to invest the energy into maintaining it just because you need to be
 stubborn. But it'd really be great if you invested that energy in a more
 productive manner ;-)
 There are two points of view. And both have con  pro.
 I don't want to argue. Net custodian should decided if is OK or not.

 Look at tsec.h and probably others.
 
 That something's in mainline doesn't mean it's obviously correct !
 

Make no sense to argue with you. Two equal solutions.


 currently, so there's possibility, in future this won't hold?
 BTW: I am also sharing rx/tx buffer descriptors for dma.

 When do you expect that u-boot will be able to use several MACs in one
 time?
 It's not a matter of when, but -- write a correct code, it's much less
 burden to fix it later.
 Agree in general.
 It is always question of when. You can always do it in better way. The
 question is if someone will pay you for doing it in better way.
 
 And if the code isn't accepted, they won't pay you ;-)

I don't need to add these drivers to mainline. For me is much easier to keep
it just for me and don't care about others.

 
 If this
 feature is not important for us, make no sense to invest our time/money to
 it.
 
 I guess having the code mainline is important ?

Partially yes. It depends how often is API changed. If API change is so often
it is better to be in mainline because others fixed your driver too. If not,
you can easily fix your driver yourself.
If you are out of mainline, you don't need to spend your time to keep it alive
on the latest version. Most of our customers don't need u-boot for final 
products too.


 +  /* Write new speed setting out to Axi Ethernet */
 +  aximac_out32(dev-iobase, XAE_EMMC_OFFSET, emmc_reg);
 Use clrsetbits() here.
 Not defined for Microblaze - just for ARM/PPC.
 Not going to use it.
 Please fix then. You're the microblaze maintainer, right?
 Custodian.
 Oh come on ...

 But I won't do that.
 I think you should.

 If you think that all archs should have it then move it to generic
 location which clean code duplication and I will include it.
 That's not the point, it's platform specific.
 Just compare ppc/arm implementation and they are the same. It is even pure
 generic code. Adding it to microblaze is just code duplication which is
 also not good way to go. Then in future someone will move it to generic
 location.
 There were a lot of examples in linux kernel and includes.
 
 Ok, if it's a generic code, please submit a patch putting it into a generic 
 place and the fix this driver to use it.

It it the same point to you.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Wolfgang Denk]

Dear Michal Simek,

In message 4e5f4883.3080...@monstr.eu you wrote:

  Well I see it both ways ... 0x4000 == 1  30 ... it's the same thing. 
  On 
  the other note, it's hard to count the zeroes in there AND you can mistake 
  0 and 
  8 in a huge series of those.
  
  Also, you can have whatever you want in your repo if you seriously care to 
  invest the energy into maintaining it just because you need to be stubborn. 
  But 
  it'd really be great if you invested that energy in a more productive 
  manner ;-)
 
 There are two points of view. And both have con  pro.
 I don't want to argue. Net custodian should decided if is OK or not.

I can parse 0x0020 much faster than '1  21' so my personal
preference for such masks is the raw hex numbers.

But this is just that: a personal preference.  I see no problem with
using either one or the other form, and I have no really strong
preferences.  In any case this should not be a reason for serious
fights - it's a matter of taste issue, not more.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
A witty saying proves nothing, but saying  something  pointless  gets
people's attention.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Marek Vasut]

On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
 Add the first axi_ethernet driver for little-endian Microblaze.
 
 Signed-off-by: Michal Simek mon...@monstr.eu
 ---
  drivers/net/Makefile  |1 +
  drivers/net/xilinx_axi_emac.c |  622
 + include/netdev.h  | 
   1 +
  3 files changed, 624 insertions(+), 0 deletions(-)
  create mode 100644 drivers/net/xilinx_axi_emac.c
 
 diff --git a/drivers/net/Makefile b/drivers/net/Makefile
 index 4541eaf..ae9d4cb 100644
 --- a/drivers/net/Makefile
 +++ b/drivers/net/Makefile
 @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
  COBJS-$(CONFIG_ULI526X) += uli526x.o
  COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
 +COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o
  COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
  COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o
 
 diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
 new file mode 100644
 index 000..ce79b80
 --- /dev/null
 +++ b/drivers/net/xilinx_axi_emac.c
 @@ -0,0 +1,622 @@
 +/*
 + * Copyright (C) 2011 Michal Simek mon...@monstr.eu
 + * Copyright (C) 2011 PetaLogix
 + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include config.h
 +#include common.h
 +#include net.h
 +#include malloc.h
 +#include asm/io.h
 +#include phy.h
 +#include miiphy.h
 +
 +/* Axi Ethernet registers offset */
 +#define XAE_IS_OFFSET0x000C /* Interrupt status */
 +#define XAE_IE_OFFSET0x0014 /* Interrupt enable */
 +#define XAE_RCW1_OFFSET  0x0404 /* Rx Configuration Word 1 */
 +#define XAE_TC_OFFSET0x0408 /* Tx Configuration */
 +#define XAE_EMMC_OFFSET  0x0410 /* EMAC mode configuration */
 +#define XAE_MDIO_MC_OFFSET   0x0500 /* MII Management Config */
 +#define XAE_MDIO_MCR_OFFSET  0x0504 /* MII Management Control */
 +#define XAE_MDIO_MWD_OFFSET  0x0508 /* MII Management Write Data */
 +#define XAE_MDIO_MRD_OFFSET  0x050C /* MII Management Read Data */

Please use struct xae_regs {...} as the rest of the u-boot.

 +
 +/* Link setup */
 +#define XAE_EMMC_LINKSPEED_MASK  0xC000 /* Link speed */
 +#define XAE_EMMC_LINKSPD_10  0x /* Link Speed mask for 10 Mbit */
 +#define XAE_EMMC_LINKSPD_100 0x4000 /* Link Speed mask for 100 Mbit */
 +#define XAE_EMMC_LINKSPD_10000x8000 /* Link Speed mask for 1000 
 Mbit
 */ +

Use (1  n) ?

 +/* Interrupt Status/Enable/Mask Registers bit definitions */
 +#define XAE_INT_RXRJECT_MASK 0x0008 /* Rx frame rejected */
 +#define XAE_INT_MGTRDY_MASK  0x0080 /* MGT clock Lock */
 +

[...]

 +#define DMAALIGN 128
 +
 +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;

Don't use cammelcase, all lowcase please. Also, can't you allocate this with 
memalign and hide it in axidma_priv or something ?
 +
 +/* reflect dma offsets */
 +struct axidma_reg {
 + u32 control; /* DMACR */
 + u32 status; /* DMASR */
 + u32 current; /* CURDESC */
 + u32 reserved;
 + u32 tail; /* TAILDESC */
 +};
 +
 +/* Private driver structures */
 +struct axidma_priv {
 + struct axidma_reg *dmatx;
 + struct axidma_reg *dmarx;
 + int phyaddr;
 +
 + struct phy_device *phydev;
 + struct mii_dev *bus;
 +};
 +
 +/* BD descriptors */
 +struct axidma_bd {
 + u32 next;   /* Next descriptor pointer */
 + u32 reserved1;
 + u32 phys;   /* Buffer address */
 + u32 reserved2;
 + u32 reserved3;
 + u32 reserved4;
 + u32 cntrl;  /* Control */
 + u32 status; /* Status */
 + u32 app0;
 + u32 app1;   /* TX start  16 | insert */
 + u32 app2;   /* TX csum seed */
 + u32 app3;
 + u32 app4;
 + u32 sw_id_offset;
 + u32 reserved5;
 + u32 reserved6;
 +};
 +
 +/* Static BDs - driver uses only one BD */
 +static struct axidma_bd tx_bd __attribute((aligned(DMAALIGN)));
 +static struct axidma_bd rx_bd __attribute((aligned(DMAALIGN)));
 +
 +static inline void aximac_out32(u32 addr, u32 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Marek Vasut wrote:
 On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
 Add the first axi_ethernet driver for little-endian Microblaze.

 Signed-off-by: Michal Simek mon...@monstr.eu
 ---
  drivers/net/Makefile  |1 +
  drivers/net/xilinx_axi_emac.c |  622
 + include/netdev.h  | 
   1 +
  3 files changed, 624 insertions(+), 0 deletions(-)
  create mode 100644 drivers/net/xilinx_axi_emac.c

 diff --git a/drivers/net/Makefile b/drivers/net/Makefile
 index 4541eaf..ae9d4cb 100644
 --- a/drivers/net/Makefile
 +++ b/drivers/net/Makefile
 @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
  COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
  COBJS-$(CONFIG_ULI526X) += uli526x.o
  COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
 +COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o
  COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
  COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o

 diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
 new file mode 100644
 index 000..ce79b80
 --- /dev/null
 +++ b/drivers/net/xilinx_axi_emac.c
 @@ -0,0 +1,622 @@
 +/*
 + * Copyright (C) 2011 Michal Simek mon...@monstr.eu
 + * Copyright (C) 2011 PetaLogix
 + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
 + *
 + * See file CREDITS for list of people who contributed to this
 + * project.
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include config.h
 +#include common.h
 +#include net.h
 +#include malloc.h
 +#include asm/io.h
 +#include phy.h
 +#include miiphy.h
 +
 +/* Axi Ethernet registers offset */
 +#define XAE_IS_OFFSET   0x000C /* Interrupt status */
 +#define XAE_IE_OFFSET   0x0014 /* Interrupt enable */
 +#define XAE_RCW1_OFFSET 0x0404 /* Rx Configuration Word 1 */
 +#define XAE_TC_OFFSET   0x0408 /* Tx Configuration */
 +#define XAE_EMMC_OFFSET 0x0410 /* EMAC mode configuration */
 +#define XAE_MDIO_MC_OFFSET  0x0500 /* MII Management Config */
 +#define XAE_MDIO_MCR_OFFSET 0x0504 /* MII Management Control */
 +#define XAE_MDIO_MWD_OFFSET 0x0508 /* MII Management Write Data */
 +#define XAE_MDIO_MRD_OFFSET 0x050C /* MII Management Read Data */
 
 Please use struct xae_regs {...} as the rest of the u-boot.

struct is not useful here because it will be big with a lot of reserved values.

 
 +
 +/* Link setup */
 +#define XAE_EMMC_LINKSPEED_MASK 0xC000 /* Link speed */
 +#define XAE_EMMC_LINKSPD_10 0x /* Link Speed mask for 10 Mbit */
 +#define XAE_EMMC_LINKSPD_1000x4000 /* Link Speed mask for 100 
 Mbit */
 +#define XAE_EMMC_LINKSPD_1000   0x8000 /* Link Speed mask for 1000 
 Mbit
 */ +
 
 Use (1  n) ?

just another solution - I prefer to use 32bit value - easier when you decode it
by md.

 
 +/* Interrupt Status/Enable/Mask Registers bit definitions */
 +#define XAE_INT_RXRJECT_MASK0x0008 /* Rx frame rejected */
 +#define XAE_INT_MGTRDY_MASK 0x0080 /* MGT clock Lock */
 +
 
 [...]

same here..

 
 +#define DMAALIGN128
 +
 +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
 
 Don't use cammelcase, all lowcase please.

Agree - will be done in v2.

  Also, can't you allocate this with
 memalign and hide it in axidma_priv or something ?

There are two things in one.
1. Adding it to axidma_priv means that I will need to dynamically allocate big 
private structure
which is bad thing to do for several eth devices. This is FPGA you can create a 
lot of MACs that's
why I think it is better not to do so.
2. Current style is sharing this rxframe buffer among all controllers because 
only one MAC works.
Others are stopped which means that no packet come to them.

BTW: Looking for that memalign function - thanks.



 +
 +/* reflect dma offsets */
 +struct axidma_reg {
 +u32 control; /* DMACR */
 +u32 status; /* DMASR */
 +u32 current; /* CURDESC */
 +u32 reserved;
 +u32 tail; /* TAILDESC */
 +};
 +
 +/* Private driver structures */
 +struct axidma_priv {
 +struct axidma_reg *dmatx;
 +struct axidma_reg *dmarx;
 +int phyaddr;
 +
 +struct phy_device *phydev;
 +struct mii_dev *bus;
 +};
 +
 +/* BD descriptors */
 +struct 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Marek Vasut]

On Wednesday, August 31, 2011 04:46:22 PM Michal Simek wrote:
 Marek Vasut wrote:
  On Tuesday, August 30, 2011 02:05:19 PM Michal Simek wrote:
  Add the first axi_ethernet driver for little-endian Microblaze.
  
  Signed-off-by: Michal Simek mon...@monstr.eu
  ---
  
   drivers/net/Makefile  |1 +
   drivers/net/xilinx_axi_emac.c |  622
  
  + include/netdev.h 
  |
  
1 +
   
   3 files changed, 624 insertions(+), 0 deletions(-)
   create mode 100644 drivers/net/xilinx_axi_emac.c
  
  diff --git a/drivers/net/Makefile b/drivers/net/Makefile
  index 4541eaf..ae9d4cb 100644
  --- a/drivers/net/Makefile
  +++ b/drivers/net/Makefile
  @@ -83,6 +83,7 @@ COBJS-$(CONFIG_TSEC_ENET) += tsec.o fsl_mdio.o
  
   COBJS-$(CONFIG_TSI108_ETH) += tsi108_eth.o
   COBJS-$(CONFIG_ULI526X) += uli526x.o
   COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
  
  +COBJS-$(CONFIG_XILINX_AXIEMAC) += xilinx_axi_emac.o
  
   COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
   COBJS-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o
  
  diff --git a/drivers/net/xilinx_axi_emac.c
  b/drivers/net/xilinx_axi_emac.c new file mode 100644
  index 000..ce79b80
  --- /dev/null
  +++ b/drivers/net/xilinx_axi_emac.c
  @@ -0,0 +1,622 @@
  +/*
  + * Copyright (C) 2011 Michal Simek mon...@monstr.eu
  + * Copyright (C) 2011 PetaLogix
  + * Copyright (C) 2010 Xilinx, Inc. All rights reserved.
  + *
  + * See file CREDITS for list of people who contributed to this
  + * project.
  + *
  + * This program is free software; you can redistribute it and/or
  + * modify it under the terms of the GNU General Public License as
  + * published by the Free Software Foundation; either version 2 of
  + * the License, or (at your option) any later version.
  + *
  + * This program is distributed in the hope that it will be useful,
  + * but WITHOUT ANY WARRANTY; without even the implied warranty of
  + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  + * GNU General Public License for more details.
  + *
  + * You should have received a copy of the GNU General Public License
  + * along with this program; if not, write to the Free Software
  + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
  + * MA 02111-1307 USA
  + */
  +
  +#include config.h
  +#include common.h
  +#include net.h
  +#include malloc.h
  +#include asm/io.h
  +#include phy.h
  +#include miiphy.h
  +
  +/* Axi Ethernet registers offset */
  +#define XAE_IS_OFFSET 0x000C /* Interrupt status */
  +#define XAE_IE_OFFSET 0x0014 /* Interrupt enable */
  +#define XAE_RCW1_OFFSET   0x0404 /* Rx Configuration Word 1 */
  +#define XAE_TC_OFFSET 0x0408 /* Tx Configuration */
  +#define XAE_EMMC_OFFSET   0x0410 /* EMAC mode configuration */
  +#define XAE_MDIO_MC_OFFSET0x0500 /* MII Management Config */
  +#define XAE_MDIO_MCR_OFFSET   0x0504 /* MII Management Control */
  +#define XAE_MDIO_MWD_OFFSET   0x0508 /* MII Management Write Data 
  */
  +#define XAE_MDIO_MRD_OFFSET   0x050C /* MII Management Read Data 
  */
  
  Please use struct xae_regs {...} as the rest of the u-boot.
 
 struct is not useful here because it will be big with a lot of reserved
 values.

I see like 10 registers here, you can use uint32_t reserved[n];

 
  +
  +/* Link setup */
  +#define XAE_EMMC_LINKSPEED_MASK   0xC000 /* Link speed */
  +#define XAE_EMMC_LINKSPD_10   0x /* Link Speed mask for 10 
  Mbit
  */ +#define XAE_EMMC_LINKSPD_100   0x4000 /* Link Speed mask for 100
  Mbit */ +#define XAE_EMMC_LINKSPD_1000 0x8000 /* Link Speed mask
  for 1000 Mbit */ +
  
  Use (1  n) ?
 
 just another solution - I prefer to use 32bit value - easier when you
 decode it by md.

It's hard to read, really.

 
  +/* Interrupt Status/Enable/Mask Registers bit definitions */
  +#define XAE_INT_RXRJECT_MASK  0x0008 /* Rx frame rejected */
  +#define XAE_INT_MGTRDY_MASK   0x0080 /* MGT clock Lock */
  +
  
  [...]
 
 same here..
 
  +#define DMAALIGN  128
  +
  +static u8 RxFrame[PKTSIZE_ALIGN] __attribute((aligned(DMAALIGN))) ;
  
  Don't use cammelcase, all lowcase please.
 
 Agree - will be done in v2.
 
   Also, can't you allocate this with
 
  memalign and hide it in axidma_priv or something ?
 
 There are two things in one.
 1. Adding it to axidma_priv means that I will need to dynamically allocate
 big private structure which is bad thing to do for several eth devices.
 This is FPGA you can create a lot of MACs that's why I think it is better
 not to do so.
 2. Current style is sharing this rxframe buffer among all controllers
 because only one MAC works. Others are stopped which means that no packet
 come to them.

Ok, I don't think I understand pt. 1. -- you need to keep the state for each of 
the ethernet devices on the FPGA separate, don't you. As for pt. 2. -- 
currently, so there's possibility, 

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, August 30, 2011 08:05:19 Michal Simek wrote:
 +static void setup_mac(struct eth_device *dev)
 +{
 + /* Set the MAC address */
 + int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
 + (dev-enetaddr[1]  8) | (dev-enetaddr[0]));
 + aximac_out32(dev-iobase, XAE_UAW0_OFFSET, val);
 +
 + val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
 + val |= aximac_in32(dev-iobase, XAE_UAW1_OFFSET) 
 + ~XAE_UAW1_UNICASTADDR_MASK;
 + aximac_out32(dev-iobase, XAE_UAW1_OFFSET, val);
 +}
 +
 +static int axiemac_init(struct eth_device *dev, bd_t * bis)
 +{
 + setup_mac(dev);

pretty sure this should be dev-write_hwaddr

 +int xilinx_axiemac_initialize(bd_t *bis, unsigned long base_addr, int
 dma_addr)

you got base_addr right, but forgot to change dma_addr to unsigned long too ;)

otherwise it seems that Marek covered much of what i would have suggested
-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] net: axi_ethernet: Add driver to u-boot

[Wolfgang Denk]

Dear Michal Simek,

In message 4e5e493e.3070...@monstr.eu you wrote:

  +/* Axi Ethernet registers offset */
  +#define XAE_IS_OFFSET 0x000C /* Interrupt status */
  +#define XAE_IE_OFFSET 0x0014 /* Interrupt enable */
  +#define XAE_RCW1_OFFSET   0x0404 /* Rx Configuration Word 1 */
  +#define XAE_TC_OFFSET 0x0408 /* Tx Configuration */
  +#define XAE_EMMC_OFFSET   0x0410 /* EMAC mode configuration */
  +#define XAE_MDIO_MC_OFFSET0x0500 /* MII Management Config */
  +#define XAE_MDIO_MCR_OFFSET   0x0504 /* MII Management Control */
  +#define XAE_MDIO_MWD_OFFSET   0x0508 /* MII Management Write Data 
  */
  +#define XAE_MDIO_MRD_OFFSET   0x050C /* MII Management Read Data 
  */
  
  Please use struct xae_regs {...} as the rest of the u-boot.
 
 struct is not useful here because it will be big with a lot of reserved 
 values.

Code based on base address + offset notation will not be accepted.
Please do use a struct as requested.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
If programming was easy, they wouldn't need something as  complicated
as a human being to do it, now would they?
   - L. Wall  R. L. Schwartz, _Programming Perl_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Friday, March 04, 2011 05:09:53 Michal Simek wrote:
 Mike Frysinger wrote:
  3. dev-init
  return -1 - if init failed
  return 0 - on success
  
  ok
  
  (here you are saying should be return # of devices)
  
  no, i think you confused initialize with init in my feedback
 
 ok. From my point of view is better do initialize only for one device
 and not to registered all device in the driver. IMHO this functionality
 should be done by board specific net function in board_eth_init.
 Not sure if this covers all cases which can happen. But anyway if the
 driver initialize several drivers with the same type then you are losing
 flexibility.

no, that isnt what i meant.  the initialize function is allowed to return more 
than one because a single part might support more than one interface.  think 
of MAC switches or multiple PHYs hooked up to a single MAC.  in neither of 
these cases should the board_eth_init func need to call a driver's initialize 
function multiple times imo.
-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] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Friday, March 04, 2011 05:09:53 Michal Simek wrote:
 To finish this discuss - here is what you think that it is correct.
 ad 2)
 return -1 - if initialize failed
 return 0 - never return
 return 0 - # of devices

oh, and to clarify on the return 0, i think it's conceivable that if a 
device might not exist, the initialize function may return 0.  whether any 
driver currently utilizes this is a different issue.
-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] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 also, you should change the hang() to return 0 in the init func.
 Are you sure return 0 which should mean success. Anything different from
 0 seems to me relevant.
 as i said, the initialize function is not returning success or
 failure. it is returning # of devices registered.  if you cannot
 register any, you should return 0.  having the boot process fail because
 of network issues doesnt make much sense when u-boot can do quite a lot
 without the network. including updating itself via other means.
 Interesting.
 1. you talked about hang() in initialize function(not dev-init) and for me
 xilinx_axiemac_initialize Initialize function is called from 
 board_eth_init which is called from eth_initialize(eth.c)

 There is this part of code
  if (board_eth_init != __def_eth_init) {
  if (board_eth_init(bis)  0)
  printf(Board Net Initialization Failed\n);

 If initialization failed the return value is  0.
 That's why hang() should be changed to return -1 and doesn't matter how
 many device there are.
 
 this detail isnt currently ironed out, so if you wanted to change the 
 hang() 
 into return -1 or return 0, that is fine.

ok.

 
 If you write:
   also, you should change the hang() to return 0 in the init func.

 (hang is only in xilinx_axiemac_initialize) and should be changed which I
 agree.

 If you propose any change which I should do, I expect that if you are focus
 on blackfin that you have done that changes in all blackfin eth drivers.
 For example in bfin_mac.c where hang is also used.
 
 incorrect code in other drivers (including bfin_mac) is not justification for 
 adding incorrect code to new drivers.  bfin_mac.c's call to hang() is wrong 
 too in the current code base.

I am not arguing that I shouldn't fix this driver. I was/am still open to any 
suggestions which help 
me to improve microblaze code/drivers.
I am just saying that if you know that there is something wrong (even in 
blackfin part) and it is 
easy to fix it, then it will be good to fix it.

 
 I maintain emaclite driver and none tell me this that's why the process
 is so slow. I believe if you release that documentation, which you are
 talking about, then others will clean/test their drivers.
 the behavior i describe isnt a decision i made.  it was made by the
 previous net maintainer and agreed upon by others in the discussion.
 Ok. If that decision was made than I expect that should be written
 somewhere in doc. I know it is boring to write any documentation but I
 expect that if any decision was made then is common that general code will
 be changed.
 
 obviously that is true, but docs only get written/updated when someone is so 
 inclined to do the work.  the existing doc only exists because i felt like 
 writing at the time.  ive never been the net maintainer and thus obligated 
 to write net documentation.  i just got tired of people doing it wrong and no 
 one knowing what should be going on.
 
 1. hang() - return -1
 
 ok
 
 2. driver initialize function (setup dev functions, driver name, priv, etc)
 return -1 - if initialize failed
 return 0 - on success
 
 no, return 1 when the device has successfully registered
 
 3. dev-init
 return -1 - if init failed
 return 0 - on success
 
 ok
 
 (here you are saying should be return # of devices)
 
 no, i think you confused initialize with init in my feedback

ok. From my point of view is better do initialize only for one device
and not to registered all device in the driver. IMHO this functionality should 
be done by
board specific net function in board_eth_init.
Not sure if this covers all cases which can happen. But anyway if the driver 
initialize several 
drivers with the same type then you are losing flexibility.
I see at least one reason why will be better not to do it:
Initialize only one device which can speedup bootup time and maybe security 
reasons.

Number of registered device can be counted through eth_register function or 
similar
If you and others like to return number of devices I will return it that's no 
problem.
But I don't want to loose flexibility which is the reason why FPGAs are great.

To finish this discuss - here is what you think that it is correct.
ad 2)
return -1 - if initialize failed
return 0 - never return
return 0 - # of devices

Is it ok for you?

 
 4. dev-recv
 return -1 - failed
 return 0 - packet not received
 return 0 - success - packet lenght
 
 return 0 is still success in the sense that there is nothing to do, but 
 that nuance doesnt matter

agree.

 
 5. dev-send
 return -1 - failed
 return 0 - success

 6. dev-write_hwaddr
 return -1 - failed
 return 0 - success
 
 ok

ok. great.
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 04:29:21 Michal Simek wrote:
 Mike Frysinger wrote:
  On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
  How does it look like phy lib u-boot support?
  
  i dont know what you mean ... how is phylib relevant to this ?  or are
  you just asking in general ?
 
 Ben wanted to create general phy lib support and remove all phy specific
 things from net drivers. I hope you see that connection because there is
 also phy part.
 If phy lib support is in progress (which probably not) then I would change
 the driver to support it.

yes, but that isnt relevant to any of the feedback ive given for this patch

  also, you should change the hang() to return 0 in the init func.
  
  Are you sure return 0 which should mean success. Anything different from
  0 seems to me relevant.
  
  as i said, the initialize function is not returning success or
  failure. it is returning # of devices registered.  if you cannot
  register any, you should return 0.  having the boot process fail because
  of network issues doesnt make much sense when u-boot can do quite a lot
  without the network. including updating itself via other means.
 
 Interesting.
 1. you talked about hang() in initialize function(not dev-init) and for me
 xilinx_axiemac_initialize Initialize function is called from 
 board_eth_init which is called from eth_initialize(eth.c)
 
 There is this part of code
   if (board_eth_init != __def_eth_init) {
   if (board_eth_init(bis)  0)
   printf(Board Net Initialization Failed\n);
 
 If initialization failed the return value is  0.
 That's why hang() should be changed to return -1 and doesn't matter how
 many device there are.

this detail isnt currently ironed out, so if you wanted to change the hang() 
into return -1 or return 0, that is fine.

 If you write:
   also, you should change the hang() to return 0 in the init func.
 
 (hang is only in xilinx_axiemac_initialize) and should be changed which I
 agree.
 
 If you propose any change which I should do, I expect that if you are focus
 on blackfin that you have done that changes in all blackfin eth drivers.
 For example in bfin_mac.c where hang is also used.

incorrect code in other drivers (including bfin_mac) is not justification for 
adding incorrect code to new drivers.  bfin_mac.c's call to hang() is wrong 
too in the current code base.

  I maintain emaclite driver and none tell me this that's why the process
  is so slow. I believe if you release that documentation, which you are
  talking about, then others will clean/test their drivers.
  
  the behavior i describe isnt a decision i made.  it was made by the
  previous net maintainer and agreed upon by others in the discussion.
 
 Ok. If that decision was made than I expect that should be written
 somewhere in doc. I know it is boring to write any documentation but I
 expect that if any decision was made then is common that general code will
 be changed.

obviously that is true, but docs only get written/updated when someone is so 
inclined to do the work.  the existing doc only exists because i felt like 
writing at the time.  ive never been the net maintainer and thus obligated 
to write net documentation.  i just got tired of people doing it wrong and no 
one knowing what should be going on.

 1. hang() - return -1

ok

 2. driver initialize function (setup dev functions, driver name, priv, etc)
 return -1 - if initialize failed
 return 0 - on success

no, return 1 when the device has successfully registered

 3. dev-init
 return -1 - if init failed
 return 0 - on success

ok

 (here you are saying should be return # of devices)

no, i think you confused initialize with init in my feedback

 4. dev-recv
 return -1 - failed
 return 0 - packet not received
 return 0 - success - packet lenght

return 0 is still success in the sense that there is nothing to do, but 
that nuance doesnt matter

 5. dev-send
 return -1 - failed
 return 0 - success
 
 6. dev-write_hwaddr
 return -1 - failed
 return 0 - success

ok
-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] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
 Mike Frysinger wrote:
 On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
 +  return 1;
 a bunch of these funcs return 1 when i'm pretty sure they should be 0
 init function is called from eth.c:eth_init(). From the code below you see
 that return can be =0.
 
 funny enough, that func in your patch is returning 0 when it should be 1.  it 
 doesnt explain why your recv/send are returning 1 when it should be 0.

for recv:
Doesn't matter what you return because there is no return value checking at all.
$ grep -rn eth_rx net/
net/eth.c:398:int eth_rx(void)
net/eth.c:433:  eth_rx();
net/net.c:480:  eth_rx();

+ I think that doesn't matter what you return.
For recv I am returning packet length as for example uli526x, ep93xx_eth and I 
believe other do.

The truth is that in README.drivers.eth is suggested to return 0 for recv.


for sending - yes, there should be 0 but why this even work.

net/net.c:252:  (void) eth_send (NetTxPacket, (pkt - NetTxPacket) + 
ARP_HDR_SIZE);
net/net.c:641:  (void) eth_send(pkt, len);
net/net.c:686:  (void) eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_HDR_SIZE 
+ len);
net/net.c:970:  (void) eth_send(NetTxPacket, (uchar *)s - NetTxPacket);
net/net.c:1452: (void) eth_send((uchar *)et, (pkt - (uchar 
*)et) + ARP_HDR_SIZE);
net/net.c:1484: (void) eth_send(NetArpWaitTxPacket, 
NetArpWaitTxPacketSize);
net/net.c:1618: (void) eth_send((uchar *)et, 
ETHER_HDR_SIZE + len);
+
api/api_net.c:100:  return eth_send(buf, len);

Only api_net checks return values.

If both functions should return 0 then any code should check it and all others 
drivers should be fixed.

 
 the init func is a special beast -- it returns the # of devices registered, 
 not 1 is success.

Where is it written?
ep93xx_eth.c returns also 1.
Anyway if is number of registered devices, 1 should means one registered 
device.
If zero means one registered device then please point me to that documentation.

Does any code even work with return value?

I expect that if I use bad return values that network driver shouldn't work but 
it works.

 
 +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
 +{
 +  struct eth_device *dev;
 +  struct axidma_priv *dma;
 +
 +  dev = malloc(sizeof(*dev));
 +  if (dev == NULL)
 +  hang();
 +
 +  memset(dev, 0, sizeof(*dev));
 +  sprintf(dev-name, Xilinx_AxiEmac);
 +
 +  dev-iobase = base_addr;
 +  dma = dev-priv;
 +  dma-dmatx = dma_addr;
 +  dma-dmarx = (u32)dma-dmatx + 0x30; /* rx channel offset */
 hmm, this is weird.  you just memset(dev) to 0, and then used dev-priv
 without assigning it storage.  so you're scribbling on top of address 0
 with your dma struct here arent you ?
 dev contains:
 iobase - axi emac baseaddr
 init, halt, send, recv pointers to functions
 and pointer priv
 + others

 I need to clear it that's why memset.

 dma controller is special IP that's why I use priv for storing pointer to
 axidma_priv structure which contains two pointers to dma for master to
 slave and slave to master directions (rx and tx if you like). As I wrote
 dma controller is different IP that's why there are different addresses.
 axi_dma has offset between channels which is 0x30.
 I used structures for hw access.
 
 ok, but i think you missed my point.  let me use this example:
 dev-priv = 0;/* the memset */
 dma = dev-priv;
 dma-dmatx = dma_addr;
 
 you're doing a NULL pointer deref here.

you are right. I have to fix it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
 If both functions should return 0 then any code should check it and all
 others drivers should be fixed.

i agree, but that doesnt mean new code should knowingly be left broken

  the init func is a special beast -- it returns the # of devices
  registered, not 1 is success.
 
 Where is it written?

atm, only the mailing list.  i have a local patch that i havent gotten around 
to pushing up yet for the README.drivers.eth.

 ep93xx_eth.c returns also 1.
 Anyway if is number of registered devices, 1 should means one registered
 device. If zero means one registered device then please point me to that
 documentation.

the change hasnt been ported to all drivers yet.  but new drivers should be 
doing it as i described.

also, you should change the hang() to return 0 in the init func.
-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] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

 +static void setup_mac(struct eth_device *dev)
 +{
 +/* Set the MAC address */
 +int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
 +(dev-enetaddr[1]  8) | (dev-enetaddr[0]));
 +out_be32(dev-iobase + XAE_UAW0_OFFSET, val);
 +
 +val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
 +val |= in_be32(dev-iobase + XAE_UAW1_OFFSET)
 + ~XAE_UAW1_UNICASTADDR_MASK;
 +out_be32(dev-iobase + XAE_UAW1_OFFSET, val);
 +}
 ...
 +static int axiemac_init(struct eth_device *dev, bd_t * bis)
 +{
 ...
 +setup_mac(dev);
 
 this should be moved to eth_device.write_hwaddr in the initialize function.  
 then the common layers will call setup_mac for you as needed.

I am not going to use write_hwaddr function because axi emac needs some 
initialization before you 
can write a mac addr that's why I will keep it in the same location as is.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote:
  +static void setup_mac(struct eth_device *dev)
  +{
  +  /* Set the MAC address */
  +  int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
  +  (dev-enetaddr[1]  8) | (dev-enetaddr[0]));
  +  out_be32(dev-iobase + XAE_UAW0_OFFSET, val);
  +
  +  val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
  +  val |= in_be32(dev-iobase + XAE_UAW1_OFFSET)
  +   ~XAE_UAW1_UNICASTADDR_MASK;
  +  out_be32(dev-iobase + XAE_UAW1_OFFSET, val);
  +}
  ...
  +static int axiemac_init(struct eth_device *dev, bd_t * bis)
  +{
  ...
  +  setup_mac(dev);
  
  this should be moved to eth_device.write_hwaddr in the initialize
  function. then the common layers will call setup_mac for you as needed.
 
 I am not going to use write_hwaddr function because axi emac needs some
 initialization before you can write a mac addr that's why I will keep it
 in the same location as is.

please add explicit comments to the setup_mac() func then so someone else 
doesnt go changing things without taking this into consideration first
-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] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
 If both functions should return 0 then any code should check it and all
 others drivers should be fixed.
 
 i agree, but that doesnt mean new code should knowingly be left broken

I agree that make no sense do not fix it right now.

 
 the init func is a special beast -- it returns the # of devices
 registered, not 1 is success.
 Where is it written?
 
 atm, only the mailing list.  i have a local patch that i havent gotten around 
 to pushing up yet for the README.drivers.eth.
 
 ep93xx_eth.c returns also 1.
 Anyway if is number of registered devices, 1 should means one registered
 device. If zero means one registered device then please point me to that
 documentation.
 
 the change hasnt been ported to all drivers yet.  but new drivers should be 
 doing it as i described.

How does it look like phy lib u-boot support?

 
 also, you should change the hang() to return 0 in the init func.

Are you sure return 0 which should mean success. Anything different from 0 
seems to me relevant.

you too.
int bfin_EMAC_initialize(bd_t *bis)
{
struct eth_device *dev;
dev = malloc(sizeof(*dev));
if (dev == NULL)
hang();


I maintain emaclite driver and none tell me this that's why the process is so 
slow.
I believe if you release that documentation, which you are talking about, then 
others will 
clean/test their drivers.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
 Mike Frysinger wrote:
 On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
 +  return 1;
 a bunch of these funcs return 1 when i'm pretty sure they should be 0
 init function is called from eth.c:eth_init(). From the code below you see
 that return can be =0.
 
 funny enough, that func in your patch is returning 0 when it should be 1.  it 
 doesnt explain why your recv/send are returning 1 when it should be 0.

BTW: blackin emac recv functions return 0, -1 and length values.
Is it correct?

static int bfin_EMAC_recv(struct eth_device *dev)
{
int length = 0;

for (;;) {
if ((rxbuf[rxIdx]-StatusWord  RX_COMP) == 0) {
length = -1;
break;
}
...
return length;
}

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote:
 +static void setup_mac(struct eth_device *dev)
 +{
 +  /* Set the MAC address */
 +  int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
 +  (dev-enetaddr[1]  8) | (dev-enetaddr[0]));
 +  out_be32(dev-iobase + XAE_UAW0_OFFSET, val);
 +
 +  val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
 +  val |= in_be32(dev-iobase + XAE_UAW1_OFFSET)
 +   ~XAE_UAW1_UNICASTADDR_MASK;
 +  out_be32(dev-iobase + XAE_UAW1_OFFSET, val);
 +}
 ...
 +static int axiemac_init(struct eth_device *dev, bd_t * bis)
 +{
 ...
 +  setup_mac(dev);
 this should be moved to eth_device.write_hwaddr in the initialize
 function. then the common layers will call setup_mac for you as needed.
 I am not going to use write_hwaddr function because axi emac needs some
 initialization before you can write a mac addr that's why I will keep it
 in the same location as is.
 
 please add explicit comments to the setup_mac() func then so someone else 
 doesnt go changing things without taking this into consideration first

No problem to write it.

I have experience with several network IPs but I think less amount than you.
But anyway I think it is common that device must be in proper state to be able 
to setup mac for most 
of them.
 From my simple test I see that dev-write_hwaddr is called before dev-init 
which looks weird.

Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
 Mike Frysinger wrote:
  On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
  If both functions should return 0 then any code should check it and all
  others drivers should be fixed.
  
  i agree, but that doesnt mean new code should knowingly be left broken
 
 I agree that make no sense do not fix it right now.

err, your new driver should return the correct values.  what higher levels do 
or do not check does not matter, and whether other drivers do it correctly 
does not matter.

  ep93xx_eth.c returns also 1.
  Anyway if is number of registered devices, 1 should means one
  registered device. If zero means one registered device then please
  point me to that documentation.
  
  the change hasnt been ported to all drivers yet.  but new drivers should
  be doing it as i described.
 
 How does it look like phy lib u-boot support?

i dont know what you mean ... how is phylib relevant to this ?  or are you 
just asking in general ?

  also, you should change the hang() to return 0 in the init func.
 
 Are you sure return 0 which should mean success. Anything different from 0
 seems to me relevant.

as i said, the initialize function is not returning success or failure.  
it is returning # of devices registered.  if you cannot register any, you 
should return 0.  having the boot process fail because of network issues 
doesnt make much sense when u-boot can do quite a lot without the network.  
including updating itself via other means.

 I maintain emaclite driver and none tell me this that's why the process is
 so slow. I believe if you release that documentation, which you are
 talking about, then others will clean/test their drivers.

the behavior i describe isnt a decision i made.  it was made by the previous 
net maintainer and agreed upon by others in the discussion.
-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] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 03:37:30 Michal Simek wrote:
 Mike Frysinger wrote:
  On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
  Mike Frysinger wrote:
  On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
  +return 1;
  
  a bunch of these funcs return 1 when i'm pretty sure they should be 0
  
  init function is called from eth.c:eth_init(). From the code below you
  see that return can be =0.
  
  funny enough, that func in your patch is returning 0 when it should be 1.
   it doesnt explain why your recv/send are returning 1 when it should be
  0.
 
 BTW: blackin emac recv functions return 0, -1 and length values.
 Is it correct?

looking at net/eth.c, -1 is an error return value.  add in current docs which 
says 0 is a success return value.  that leaves all other values as currently 
undocumented.  if a driver survey suggested that returning the length was 
useful info, then perhaps the documentation could be updated.  but as you 
noted, that return value isnt currently checked.

so for now, you could change the recv func to return the length rather than 0 
and i wouldnt complain, but always returning 1 is wrong.
-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] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Tuesday, March 01, 2011 03:46:16 Michal Simek wrote:
 Mike Frysinger wrote:
  On Tuesday, March 01, 2011 03:19:12 Michal Simek wrote:
  +static void setup_mac(struct eth_device *dev)
  +{
  +/* Set the MAC address */
  +int val = ((dev-enetaddr[3]  24) | (dev-enetaddr[2]  16) |
  +(dev-enetaddr[1]  8) | (dev-enetaddr[0]));
  +out_be32(dev-iobase + XAE_UAW0_OFFSET, val);
  +
  +val = (dev-enetaddr[5]  8) | dev-enetaddr[4] ;
  +val |= in_be32(dev-iobase + XAE_UAW1_OFFSET)
  + 
  ~XAE_UAW1_UNICASTADDR_MASK;
  +out_be32(dev-iobase + XAE_UAW1_OFFSET, val);
  +}
  ...
  +static int axiemac_init(struct eth_device *dev, bd_t * bis)
  +{
  ...
  +setup_mac(dev);
  
  this should be moved to eth_device.write_hwaddr in the initialize
  function. then the common layers will call setup_mac for you as needed.
  
  I am not going to use write_hwaddr function because axi emac needs some
  initialization before you can write a mac addr that's why I will keep it
  in the same location as is.
  
  please add explicit comments to the setup_mac() func then so someone else
  doesnt go changing things without taking this into consideration first
 
 No problem to write it.
 
 I have experience with several network IPs but I think less amount than
 you.

i think you unduly credit me with more experience than i actually have

 But anyway I think it is common that device must be in proper state
 to be able to setup mac for most of them.
  From my simple test I see that dev-write_hwaddr is called before
 dev-init which looks weird.

i dont think this is weird ... usually the MAC address is simply memory mapped 
storage, so the state of the EMAC controller itself doesnt matter.  you can 
peek/poke the contents of that MAC address storage at any time.

but i dont know what weirdness your controller has in place to make this not 
work too.
-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] net: axi_ethernet: Add driver to u-boot

[Michal Simek]

Mike Frysinger wrote:
 On Tuesday, March 01, 2011 03:34:38 Michal Simek wrote:
 Mike Frysinger wrote:
 On Tuesday, March 01, 2011 03:00:47 Michal Simek wrote:
 If both functions should return 0 then any code should check it and all
 others drivers should be fixed.
 i agree, but that doesnt mean new code should knowingly be left broken
 I agree that make no sense do not fix it right now.
 
 err, your new driver should return the correct values.  what higher levels do 
 or do not check does not matter, and whether other drivers do it correctly 
 does not matter.

Comment below.

 
 ep93xx_eth.c returns also 1.
 Anyway if is number of registered devices, 1 should means one
 registered device. If zero means one registered device then please
 point me to that documentation.
 the change hasnt been ported to all drivers yet.  but new drivers should
 be doing it as i described.
 How does it look like phy lib u-boot support?
 
 i dont know what you mean ... how is phylib relevant to this ?  or are you 
 just asking in general ?

Ben wanted to create general phy lib support and remove all phy specific things 
from net drivers.
I hope you see that connection because there is also phy part.
If phy lib support is in progress (which probably not) then I would change the 
driver to support it.

 
 also, you should change the hang() to return 0 in the init func.
 Are you sure return 0 which should mean success. Anything different from 0
 seems to me relevant.
 
 as i said, the initialize function is not returning success or failure.  
 it is returning # of devices registered.  if you cannot register any, you 
 should return 0.  having the boot process fail because of network issues 
 doesnt make much sense when u-boot can do quite a lot without the network.  
 including updating itself via other means.

Interesting.
1. you talked about hang() in initialize function(not dev-init) and for me 
xilinx_axiemac_initialize
Initialize function is called from  board_eth_init which is called from 
eth_initialize(eth.c)

There is this part of code
if (board_eth_init != __def_eth_init) {
if (board_eth_init(bis)  0)
printf(Board Net Initialization Failed\n);

If initialization failed the return value is  0.
That's why hang() should be changed to return -1 and doesn't matter how many 
device there are.

If you write:
  also, you should change the hang() to return 0 in the init func.
(hang is only in xilinx_axiemac_initialize) and should be changed which I agree.

If you propose any change which I should do, I expect that if you are focus on 
blackfin that you 
have done that changes in all blackfin eth drivers. For example in bfin_mac.c 
where hang is also used.


 
 I maintain emaclite driver and none tell me this that's why the process is
 so slow. I believe if you release that documentation, which you are
 talking about, then others will clean/test their drivers.
 
 the behavior i describe isnt a decision i made.  it was made by the previous 
 net maintainer and agreed upon by others in the discussion.

Ok. If that decision was made than I expect that should be written somewhere in 
doc.
I know it is boring to write any documentation but I expect that if any 
decision was made then is 
common that general code will be changed. The changes can be from top to down. 
If general eth code 
checks return values then driver which returns wrong return codes won't work.

I am not arguing that my driver shouldn't return correct values!!! That's not 
my point.

If you start arguing that my driver has some faults, I am open to fix it and I 
really appreciate 
your comments.

I think it is good time to summarize all that changes we discuss: (Please 
correct it if you think 
that it is wrong). (Our communication is getting messy a little bit)

1. hang() - return -1

2. driver initialize function (setup dev functions, driver name, priv, etc)
return -1 - if initialize failed
return 0 - on success

3. dev-init
return -1 - if init failed
return 0 - on success
(here you are saying should be return # of devices)

4. dev-recv
return -1 - failed
return 0 - packet not received
return 0 - success - packet lenght

5. dev-send
return -1 - failed
return 0 - success

6. dev-write_hwaddr
return -1 - failed
return 0 - success

Thanks,
Michal



Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] net: axi_ethernet: Add driver to u-boot

[Mike Frysinger]

On Monday, February 28, 2011 13:50:55 Michal Simek wrote:
 Mike Frysinger wrote:
  On Monday, February 28, 2011 04:40:33 Michal Simek wrote:
  +  return 1;
  
  a bunch of these funcs return 1 when i'm pretty sure they should be 0
 
 init function is called from eth.c:eth_init(). From the code below you see
 that return can be =0.

funny enough, that func in your patch is returning 0 when it should be 1.  it 
doesnt explain why your recv/send are returning 1 when it should be 0.

the init func is a special beast -- it returns the # of devices registered, 
not 1 is success.

  +int xilinx_axiemac_initialize(bd_t *bis, int base_addr, int dma_addr)
  +{
  +  struct eth_device *dev;
  +  struct axidma_priv *dma;
  +
  +  dev = malloc(sizeof(*dev));
  +  if (dev == NULL)
  +  hang();
  +
  +  memset(dev, 0, sizeof(*dev));
  +  sprintf(dev-name, Xilinx_AxiEmac);
  +
  +  dev-iobase = base_addr;
  +  dma = dev-priv;
  +  dma-dmatx = dma_addr;
  +  dma-dmarx = (u32)dma-dmatx + 0x30; /* rx channel offset */
  
  hmm, this is weird.  you just memset(dev) to 0, and then used dev-priv
  without assigning it storage.  so you're scribbling on top of address 0
  with your dma struct here arent you ?
 
 dev contains:
 iobase - axi emac baseaddr
 init, halt, send, recv pointers to functions
 and pointer priv
 + others
 
 I need to clear it that's why memset.
 
 dma controller is special IP that's why I use priv for storing pointer to
 axidma_priv structure which contains two pointers to dma for master to
 slave and slave to master directions (rx and tx if you like). As I wrote
 dma controller is different IP that's why there are different addresses.
 axi_dma has offset between channels which is 0x30.
 I used structures for hw access.

ok, but i think you missed my point.  let me use this example:
dev-priv = 0;  /* the memset */
dma = dev-priv;
dma-dmatx = dma_addr;

you're doing a NULL pointer deref here.
-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