[U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch

[Joe Hershberger]

Hi Codrin,

On Wed, Jul 1, 2015 at 9:31 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu at freescale.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
>> Sent: Wednesday, July 01, 2015 1:26 AM
>> To: Ciubotariu Codrin Constantin-B43658
>> Cc: u-boot; Joe Hershberger; Sun York-R58495
>> Subject: Re: [U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default
>> configuration for VSC9953 L2 Switch
>>
>> Hi Codrin,
>>
>> On Tue, Jun 30, 2015 at 2:51 AM, Codrin Constantin Ciubotariu
>> <codrin.ciubotariu at freescale.com> wrote:
>> > Hi Joe,
>> >
>> > I removed the lines on which we agreed on...
>> >
>> >> >> > +       switch (mode) {
>> >> >> > +       case EGRESS_UNTAG_ALL:
>> >> >> > +               clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_MASK,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_NONE);
>> >> >> > +               break;
>> >> >> > +       case EGRESS_UNTAG_PVID_AND_ZERO:
>> >> >> > +               clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_MASK,
>> >> >> > +
>> >> >> > + CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO);
>> >> >>
>> >> >> This seems like the naming is inverted. The enum value is called
>> >> >> "untag" pvid and zero, but the config is called "tag" all pvid and
>> >> >> zero. Is this a bug or just poorly named constants / enum values?
>> >> >>
>> >> >> > +               break;
>> >> >> > +       case EGRESS_UNTAG_ZERO:
>> >> >> > +               clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_MASK,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_ALL_ZERO);
>> >> >>
>> >> >> Also here.
>> >> >>
>> >> >> > +               break;
>> >> >> > +       case EGRESS_UNTAG_NONE:
>> >> >> > +               clrsetbits_le32(&l2rew_reg->port[port_no].port_tag_cfg,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_MASK,
>> >> >> > +                               CONFIG_VSC9953_TAG_CFG_ALL);
>> >> >> > +               break;
>> >> >> > +       default:
>> >> >> > +               printf("Unknown untag mode for port %d\n", port_no);
>> >> >> > +       }
>> >> >
>> >> > Yes, the naming is inverted. The main reason for this is that I
>> >> > couldn't find a short and easy to use command to configure a port's
>> >> > egress to send all frames VLAN tagged except when the VLAN ID equals the
>> Port
>> >> VLAN ID.
>> >> > I decided to make a command to tell the switch for which VLAN ID's not
>> >> > to tag a frame (untag) instead of making a command to tell the switch
>> >> > for which VLAN IDs to tag the frame (tag). So, for example, the
>> >> > command "ethsw [port <port_no>] tag all except pvid" or "ethsw [port
>> >> > <port_no>] tag !pvid" became "ethsw [port <port_no>] untagged pvid".
>> >> > If you think this is not intuitive for both users and developers, I
>> >> > will try to find something more appropriate.
>> >>
>> >> I don't have a problem with using the inverted logic if that's what typical
>> use-
>> >> cases call for, what I was referring to was those two specific examples. The
>> >> "all" and "none" seem correctly inverted.
>> >>
>> >> In the other 2 cases, the "tag" vs "untag" is inverted, but the subject is
>> not
>> >> "PVID_AND_ZERO" vs "ALL_PVID_ZERO"
>> >>
>> >> "EGRESS_UNTAG_PVID_AND_ZERO" ->
>> >> "CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO", for example. That's the discrepancy
>> I'm
>> >> concerned about.
>> >
>> > Ok, should I rename the constants to something like
>> > VSC9953_TAG_CFG_ALL_BUT_PRIV_ZERO instead of
>> > CONFIG_VSC9953_TAG_CFG_ALL_PVID_ZERO and
>> > VSC9953_TAG_CFG_ALL_BUT_ZERO instead of
>> > CONFIG_VSC9953_TAG_CFG_ALL_ZERO?
>> >
>>
>> I assume you meant to say VSC9953_TAG_CFG_ALL_BUT_*PVID*_ZERO here.
>>
>> If so, I think that's clear enough.
>
> Yes, VSC9953_TAG_CFG_ALL_BUT_PVID_ZERO and VSC9953_TAG_CFG_ALL_BUT_ZERO.

Sounds good.


>> >> >> > +#define field_set(val, mask)           ((val) * ((mask) & ~((mask) <<
>> 1)))
>> >> >> > +#define field_get(val, mask)           ((val) / ((mask) & ~((mask) <<
>> 1)))
>> >> >>
>> >> >> I don't follow why this is unique to this chip? Also, get is never
>> >> >> used. Is it just for completeness, I assume.
>> >> >>
>> >> >> I think you should either be using the functions in
>> >> >> include/bitfield.h or you should be adding these there instead of
>> >> >> here. If you decide to add them there, then naturally do it as a
>> >> >> separate patch and with good comments and naming consistent with that
>> >> >> file and as functions not macros. This method is nice in that you use
>> >> >> the mask to define the shift instead of requiring it as a separate
>> constant.
>> >> >
>> >> > These are not unique to this chip. If you consider them useful, I will
>> >> > make a separate patch and add them (or something similar) to
>> >> > include/bitfield.h .
>> >>
>> >> I think this would be the best approach.
>> >
>> > Ok, I will make another patch and add bitfield_set/get() inline functions in
>> include/bitfield.h .
>>
>> I would recommend you structure it as 3 new functions.
>>
>> diff --git a/include/bitfield.h b/include/bitfield.h
>> index ec4815c..b685804 100644
>> --- a/include/bitfield.h
>> +++ b/include/bitfield.h
>> @@ -39,6 +39,12 @@ static inline uint bitfield_mask(uint shift, uint width)
>>         return ((1 << width) - 1) << shift;
>>  }
>>
>> +/* Produces a shift of the bitfield given a mask */
>> +static inline uint bitfield_shift(uint mask)
>> +{
>> +       return ffs(mask) - 1;
>> +}
>
> Ok, should we assure we return 0 if mask is 0? Something like return mask : ffs(mask) - 1 ? 0;

Sounds like a good idea.

>> +
>>  /* Extract the value of a bitfield found within a given register value */
>>  static inline uint bitfield_extract(uint reg_val, uint shift, uint width)
>>  {
>> @@ -56,3 +62,23 @@ static inline uint bitfield_replace(uint reg_val,
>> uint shift, uint width,
>>
>>         return (reg_val & ~mask) | (bitfield_val << shift);
>>  }
>> +
>> +/* Extract the value of a bitfield found within a given register value */
>> +static inline uint bitfield_extract_by_mask(uint reg_val, uint mask)
>> +{
>> +       uint shift = bitfield_shift(mask);
>> +
>> +       return (reg_val & mask) >> shift;
>> +}
>> +
>> +/*
>> + * Replace the value of a bitfield found within a given register value
>> + * Returns the newly modified uint value with the replaced field.
>> + */
>> +static inline uint bitfield_replace_by_mask(uint reg_val, uint mask,
>> +                                           uint bitfield_val)
>> +{
>> +       uint shift = bitfield_shift(mask);
>> +
>> +       return (reg_val & ~mask) | ((bitfield_val << shift) & mask);
>> +}
>
> Ok.
>
> Best regards,
> Codrin

Cheers,
-Joe