Hi Joe,
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershber...@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.ciubota...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
