Hi Joe, > -----Original Message----- > From: Joe Hershberger [mailto:joe.hershber...@gmail.com] > Sent: Wednesday, July 01, 2015 1:50 AM > To: Ciubotariu Codrin Constantin-B43658 > Cc: u-boot; Joe Hershberger; Sun York-R58495 > Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to > manipulate the FDB for VSC9953 > > Hi Codrin, > > On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu > <codrin.ciubota...@freescale.com> wrote: > > Hi Joe, > > > >> -----Original Message----- > >> From: Joe Hershberger [mailto:joe.hershber...@gmail.com] > >> Sent: Friday, June 26, 2015 1:39 AM > >> To: Ciubotariu Codrin Constantin-B43658 > >> Cc: u-boot; Joe Hershberger; Sun York-R58495 > >> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to > >> manipulate the FDB for VSC9953 > >> > >> > + return !!timeout; > >> > >> Maybe return -EBUSY like suggested in previous patch. > > > > Ok. > > > >> > + /* write port and vid to get selected FDB entries */ > >> > + val = in_le32(&l2ana_reg->ana.anag_efil); > >> > + if (port_no != VSC9953_CMD_PORT_ALL) { > >> > + val = (val & ~CONFIG_VSC9953_AGE_PORT_MASK) | > >> > + CONFIG_VSC9953_AGE_PORT_EN | > >> > + field_set(port_no, > >> > + CONFIG_VSC9953_AGE_PORT_MASK); > >> > >> Seems like a good place to use bitfield_replace() from > >> include/bitfield.h (or a new one that you add that uses the mask for > >> the shift). > >> > >> > + } > >> > + if (vid != VSC9953_CMD_VLAN_ALL) { > >> > + val = (val & ~CONFIG_VSC9953_AGE_VID_MASK) | > >> > + CONFIG_VSC9953_AGE_VID_EN | > >> > + field_set(vid, > CONFIG_VSC9953_AGE_VID_MASK); > >> > >> Same here. > > > > Ok. > > > >> > + vlan = field_get(val & CONFIG_VSC9953_MAC_VID_MASK, > >> > + CONFIG_VSC9953_MAC_VID_MASK); > >> > >> It seems like masking off the val before shifting it would be better > >> implemented inside the field_get function (renamed and moved to > >> include/bitfield.h) instead of on each use. > > > > Yes, something like #define field_set(val, mask) (((val) * > > ((mask) & > ~((mask) << 1))) & mask) and > > #define field_get(val, mask) ((val & mask) / ((mask) & ~((mask) << > 1))). > > The set seems the same as replace, just without any other bitfields to > preserve. You could just use replace and pass in 0 as the reg_val.
Yes, although I still have to pass the "shift" parameter. > > >> > + out_le32(&l2ana_reg->ana_tables.mach_data, > >> > + (mac[0] << 8) | (mac[1] << 0) | > >> > + (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) & > >> > + CONFIG_VSC9953_MACHDATA_VID_MASK)); > >> > >> Why do you need to & with the mask again after field_set()? > > > > To assure that the shifted vid value is not higher than its mask. Adding the > mask to the macro/inline function as described above should assure this. > > OK. Maybe the existing bitfield_replace() should be updated to also > mask off the bitfield_val. Yes, I think it should be updated. Should I make a separate patch for it? It should be out of this patch set since it has nothing to do with VSC9953. > > >> > + /* check if MAC address is present */ > >> > + if (!parsed_cmd->mac_addr) { > >> > >> Use this: > >> + if (is_valid_ethaddr(parsed_cmd->mac_addr)) { > > > > is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, > > but > for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()? > > I'm not sure what criteria you intend to check for here, so I'm not > sure if that is appropriate. When a command for adding/removing a mac address is issued, we should check if there is a valid mac address in parsed_cmd->mac_addr. I guess that this could be checked only in keyword_match_mac_addr(). Also, since mac_addr will become a static array, I should initialize this array with an invalid mac address. Since 0 mac address is a valid address for the L2 Switch, I was thinking to initialize it with L2 Broadcast address. What do you think? > > >> > + /* check if MAC address is present */ > >> > + if (!parsed_cmd->mac_addr) { > >> > >> Use this: > >> + if (is_valid_ethaddr(parsed_cmd->mac_addr)) { > > > > Same as above. > > > >> > +/* check if the string has the format for a MAC address */ > >> > +static int string_is_mac_addr(const char *mac) > >> > +{ > >> > + int i; > >> > + > >> > + if (!mac) > >> > + return 0; > >> > + > >> > + for (i = 0; i < 6; i++) { > >> > + if (!isxdigit(*mac) || !isxdigit(*(mac + 1))) > >> > + return 0; > >> > + mac += 2; > >> > + if (i != 5) { > >> > + if (*mac != ':' && *mac != '-') > >> > + return 0; > >> > + mac++; > >> > + } > >> > + } > >> > + > >> > + if (*mac != '\0') > >> > + return 0; > >> > + > >> > + return 1; > >> > >> This functionality is already implemented in common/env_flags.c in the > >> _env_flags_validate_type() function. Maybe that implementation should > >> be extracted. Another option is to make the eth_parse_enetaddr() in > >> net/eth.c return a status and then it can be used. Either way I don't > >> think it should be re-implemented here. > > > > Yes, I noticed that this function is already implemented, but with no access > to it. I will see how I can use the one available. > > Which path do you plan to take? I looked through both eth_parse_enetaddr() and _env_flags_validate_type() and I prefer the implementation from the latter function, since eth_parse_enetaddr() doesn't check for ':' or if the value returned by simple_strtoul() is <= 0xFF. I could move the code from "case env_flags_vartype_macaddr:" to a separate function (something like eth_check_enetaddr()? ) and add its prototype somewhere in include/net.h. What do you think? Best regards, Codrin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot