Hi Codrin,
On Thu, Jul 2, 2015 at 3:44 AM, Codrin Constantin Ciubotariu
<codrin.ciubota...@freescale.com> wrote:
> 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.
Not if you use the new bitfield_replace_by_mask().
>>
>> >> > + 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.
Definitely a separate patch.
>> >> > + /* 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?
Sounds good.
>> >> > + /* 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?
I think it should be called eth_validate_ethaddr_str(). You can add it
to include/net.h and the top of net/eth.c
Cheers,
-Joe
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
