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

[Codrin Constantin Ciubotariu]

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?

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

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