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))).
> > + 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.
> > + /* check if the MAC address was indeed added */
> > + 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()?
Same here.
> > + 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()?
Same here.
> > + 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()?
Same here.
> > + 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()?
Same here.
> > + 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).
Ok.
>
> > + }
> > +
> > + 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.
> > + uchar *mac_addr;
>
> Use this:
> + uchar ethaddr[6];
> I recently made a pass through U-Boot trying to standardize on that
> naming. Also, don't make it a pointer that has to be allocated. It is
> small and of known size.
Ok.
> > +#define VSC9953_FDB_HELP "ethsw [port <port_no>] [vlan <vid>] fdb " \
> > +"{ [help] | show | flush | { add | del } <mac> } " \
> > +"- Add/delete a mac entry in FDB; use show to see FDB entries; " \
> > +"if vlan <vid> is missing, will be used VID 1"
>
> Please use this:
> +"if vlan <vid> is missing, VID 1 will be used"
Ok.
> > + /* 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()?
> > + /* 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.
> > + parsed_cmd->mac_addr = malloc(6);
>
> Why malloc this? It is small and known size.
I will declare the array statically.
> > + if (is_broadcast_ethaddr(parsed_cmd->mac_addr)) {
> > + free(parsed_cmd->mac_addr);
> > + parsed_cmd->mac_addr = NULL;
>
> Drop these two lines.
Ok.
> > - /* Nothing to do for now */
> > + /* free MAC address */
> > + if (parsed_cmd->mac_addr) {
> > + free(parsed_cmd->mac_addr);
> > + parsed_cmd->mac_addr = NULL;
> > + }
>
> Don't malloc, and you don't need free.
Ok.
> > #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff
> > #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004
> >
> > +/* Macros for register vsc9953_ana_ana_tables.mac_access register */
> > +#define CONFIG_VSC9953_MAC_CMD_IDLE 0x00000000
> > +#define CONFIG_VSC9953_MAC_CMD_LEARN 0x00000001
> > +#define CONFIG_VSC9953_MAC_CMD_FORGET 0x00000002
> > +#define CONFIG_VSC9953_MAC_CMD_AGE 0x00000003
> > +#define CONFIG_VSC9953_MAC_CMD_NEXT 0x00000004
> > +#define CONFIG_VSC9953_MAC_CMD_READ 0x00000006
> > +#define CONFIG_VSC9953_MAC_CMD_WRITE 0x00000007
> > +#define CONFIG_VSC9953_MAC_CMD_MASK 0x00000007
> > +#define CONFIG_VSC9953_MAC_CMD_VALID 0x00000800
> > +#define CONFIG_VSC9953_MAC_ENTRYTYPE_NORMAL 0x00000000
> > +#define CONFIG_VSC9953_MAC_ENTRYTYPE_LOCKED 0x00000200
> > +#define CONFIG_VSC9953_MAC_ENTRYTYPE_IPV4MCAST 0x00000400
> > +#define CONFIG_VSC9953_MAC_ENTRYTYPE_IPV6MCAST 0x00000600
> > +#define CONFIG_VSC9953_MAC_ENTRYTYPE_MASK 0x00000600
> > +#define CONFIG_VSC9953_MAC_DESTIDX_MASK 0x000001f8
> > +#define CONFIG_VSC9953_MAC_VID_MASK 0x1fff0000
> > +#define CONFIG_VSC9953_MAC_MACH_MASK 0x0000ffff
> > +
> > /* Macros for vsc9953_ana_port.vlan_cfg register */
> > #define CONFIG_VSC9953_VLAN_CFG_AWARE_ENA 0x00100000
> > #define CONFIG_VSC9953_VLAN_CFG_POP_CNT_MASK 0x000c0000
> > @@ -131,6 +150,15 @@
> > #define CONFIG_VSC9953_TAG_CFG_ALL_ZERO 0x00000100
> > #define CONFIG_VSC9953_TAG_CFG_ALL 0x00000180
> >
> > +/* Macros for vsc9953_ana_ana.anag_efil register */
> > +#define CONFIG_VSC9953_AGE_PORT_EN 0x00080000
> > +#define CONFIG_VSC9953_AGE_PORT_MASK 0x0007c000
> > +#define CONFIG_VSC9953_AGE_VID_EN 0x00002000
> > +#define CONFIG_VSC9953_AGE_VID_MASK 0x00001fff
> > +
> > +/* Macros for vsc9953_ana_ana_tables.mach_data register */
> > +#define CONFIG_VSC9953_MACHDATA_VID_MASK 0x1fff0000
>
> Drop "CONFIG_" from all these.
Ok.
Thanks and best regards,
Codrin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
