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

[Codrin Constantin Ciubotariu]

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.

> 
> >>
> >> >> > +#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;

> +
>  /* 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