
Hi Codrin,
On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@freescale.com wrote:
Hi Joe,
-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@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.
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.
/* 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.
/* 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?
Thanks, -Joe