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

[Joe Hershberger]

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.

>>
>> >> > +#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;
+}
+
 /* 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);
+}


>> >> > +struct vsc9953_rew_common {
>> >> > +       u32     reserve[4];
>> >> > +       u32     dscp_remap_dp1_cfg[64];
>> >> > +       u32     dscp_remap_cfg[64];
>> >> > +};
>> >> > +
>> >> > +struct vsc9953_rew_reg {
>> >> > +       struct vsc9953_rew_port port[12];
>> >> > +       struct vsc9953_rew_common common; };
>> >> > +
>> >> > +/* END VSC9953 REW structure for T1040 U-boot*/
>> >>
>> >> These comments seem gratuitous and not particularly relevant (begin
>> >> and end). Perhaps either remove them throughout the file or at least
>> >> don't add more. At the very least, drop the " structure for T1040
>> >> U-boot" which isn't helpful or accurate.
>> >>
>> >
>> > Yes, the " structure for T1040 U-boot" seems irrelevant indeed. I will
>> > also remove the other comments if you consider them unnecessary. To me
>> > it looks like it groups the structures a bit and might help developers
>> > look for a specific register. I will remove them in the patch with the
>> > clean-up.
>>
>> If you think the bracketing of these structs adds clarity, then only remove the
>> trailing text. Otherwise remove all of them completely. Up to you; I'm fine with
>> either way.
>
> Ok, I will remove the trailing text and I will see if the remaining comments make sense.
>
>>
>> Thanks,
>> -Joe
>
> Thanks and best regards,
> Codrin

Cheers,
-Joe