From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Warren Date: Wed, 07 Apr 2010 10:30:59 -0700 Subject: [U-Boot] [PATCH v4 tabify] net: add altera triple speeds ethernet mac driver In-Reply-To: <4BBC03A0.4060006@wytron.com.tw> References: <1269412366-9206-1-git-send-email-thomas@wytron.com.tw> <1269765146-21288-1-git-send-email-thomas@wytron.com.tw> <4BB980E2.2040901@gmail.com> <4BBC03A0.4060006@wytron.com.tw> Message-ID: <4BBCC153.1020908@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Thomas, On 4/6/2010 9:01 PM, Thomas Chou wrote: > Hi Ben, > > Thanks. > > On 04/05/2010 02:19 PM, Ben Warren wrote: >> >>> + >>> +static int tse_eth_send(struct eth_device *dev, volatile void *packet, >>> + int length); >>> +static int tse_eth_rx(struct eth_device *dev); >>> +static void tse_eth_halt(struct eth_device *dev); >>> +static void tse_eth_reset(struct eth_device *dev); >>> +static int tse_eth_init(struct eth_device *dev, bd_t *bd); >>> + >>> +static int tse_mdio_read(struct altera_tse_priv *priv, unsigned int >>> regnum); >>> +static int tse_mdio_write(struct altera_tse_priv *priv, unsigned >>> int regnum, >>> + unsigned int value); >> >> Are these prototypes really needed? If so, please re-order the code >> so they're not. > OK. I will reorder the code so that they will be not needed. > >>> >>> +/* This is a generic routine that the SGDMA mode-specific routines >>> + * call to populate a descriptor. >>> + * arg1 :pointer to first SGDMA descriptor. >>> + * arg2 :pointer to next SGDMA descriptor. >>> + * arg3 :Address to where data to be written. >>> + * arg4 :Address from where data to be read. >>> + * arg5 :no of byte to transaction. >>> + * arg6 :variable indicating to generate start of packet or not >>> + * arg7 :read fixed >>> + * arg8 :write fixed >>> + * arg9 :read burst >>> + * arg10 :write burst >>> + * arg11 :atlantic_channel number >>> + */ >> 11 arguments??? Seriously??? > It might be simpler if I fold this call into the callers. >>> >>> +/* TSE init code */ >>> +int altera_tse_init(bd_t *bis, int num_tses) >> The naming convention that we use is xxx_initialize(), or >> xxx_register(), although I prefer the former. If you're not using >> *bis, don't pass it in. > >>> >>> + for (num = 0; num< num_tses; num++) { >> You don't use the 'num' variable. As such, this driver doesn't >> support more than one instance. Once you add true multi-instance >> support, the preferred way to do this is to call this function for >> each instance, passing in the appropriate addressing information. > This driver needs several components and several base addresses. Can I > pass them in a structure? > > int altera_tse_initialize(u8 dev_num, void *base_info) > There's precedent for that (tsec, for example), but I'm not a big fan. If it's a small number (maybe no more than 4), pass each as an argument. If you need a struct, though, you know the type, so don't use void*. >>> >>> + return num_tses; >> This return value is meaningless, as mentioned above. > Will return 0. > No, return the number of interfaces initialized. In your case, it will be 1, although I'd prefer to see this as a true multi-capable driver. >>> >>> + >>> + /* Set the MAC address */ >>> + debug("Setting MAC address to 0x%x%x%x%x%x%x\n", >>> + dev->enetaddr[5], dev->enetaddr[4], >>> + dev->enetaddr[3], dev->enetaddr[2], >>> + dev->enetaddr[1], dev->enetaddr[0]); >>> + mac_dev->mac_addr_0 = ((dev->enetaddr[3])<< 24 | >>> + (dev->enetaddr[2])<< 16 | >>> + (dev->enetaddr[1])<< 8 | (dev->enetaddr[0])); >>> + >>> + mac_dev->mac_addr_1 = ((dev->enetaddr[5]<< 8 | >>> + (dev->enetaddr[4]))& 0xFFFF); >>> + >>> + /* Set the MAC address */ >>> + mac_dev->supp_mac_addr_0_0 = mac_dev->mac_addr_0; >>> + mac_dev->supp_mac_addr_0_1 = mac_dev->mac_addr_1; >>> + >>> + /* Set the MAC address */ >>> + mac_dev->supp_mac_addr_1_0 = mac_dev->mac_addr_0; >>> + mac_dev->supp_mac_addr_1_1 = mac_dev->mac_addr_1; >>> + >>> + /* Set the MAC address */ >>> + mac_dev->supp_mac_addr_2_0 = mac_dev->mac_addr_0; >>> + mac_dev->supp_mac_addr_2_1 = mac_dev->mac_addr_1; >>> + >>> + /* Set the MAC address */ >>> + mac_dev->supp_mac_addr_3_0 = mac_dev->mac_addr_0; >>> + mac_dev->supp_mac_addr_3_1 = mac_dev->mac_addr_1; >>> + >> Please put the MAC address programming code in a separate function, >> taking a eth_dev * as parameter. It may save you work later. > OK. > >>> >>> /* Driver initialization prototypes */ >>> int au1x00_enet_initialize(bd_t*); >>> +int altera_tse_init(bd_t *bis, int num_tses); >> Alphabetical order, please. > OK. >>> int at91emac_register(bd_t *bis, unsigned long iobase); >>> int bfin_EMAC_initialize(bd_t *bis); >>> int cs8900_initialize(u8 dev_num, int base_addr); >> > > Best regards, > Thomas regards, Ben