[U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch
Codrin Constantin Ciubotariu
codrin.ciubotariu at freescale.com
Mon Jun 29 18:06:13 CEST 2015
Hi Joe,
> > + return !!timeout;
>
> Maybe this:
>
> + return timeout ? 0 : -EBUSY;
Ok.
> > +
> > + if (!vsc9953_vlan_table_poll_idle()) {
>
> If you accept the above, you need to change these to:
> + if (vsc9953_vlan_table_poll_idle() < 0) {
>
> or
>
> + if (vsc9953_vlan_table_poll_idle() == -EBUSY) {
>
Ok, I think I will go with the first choice.
> > + if (!vsc9953_vlan_table_poll_idle()) {
>
> Here too.
Ok.
> > + if (!set)
> > + clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
> > + CONFIG_VSC9953_VLAN_PORT_MASK |
> > + CONFIG_VSC9953_VLAN_CMD_MASK,
> > + field_set(CONFIG_VSC9953_VLAN_CMD_WRITE,
> > + CONFIG_VSC9953_VLAN_CMD_MASK));
> > + else
> > + clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
> > + CONFIG_VSC9953_VLAN_PORT_MASK |
> > + CONFIG_VSC9953_VLAN_CMD_MASK,
> > + field_set(CONFIG_VSC9953_VLAN_CMD_WRITE,
> > + CONFIG_VSC9953_VLAN_CMD_MASK) |
> > + CONFIG_VSC9953_VLAN_PORT_MASK);
>
> It seems this could if statement could all be simplified as:
> + clrsetbits_le32(&l2ana_reg->ana_tables.vlan_access,
> + CONFIG_VSC9953_VLAN_PORT_MASK |
> + CONFIG_VSC9953_VLAN_CMD_MASK,
> + field_set(CONFIG_VSC9953_VLAN_CMD_WRITE,
> + CONFIG_VSC9953_VLAN_CMD_MASK) |
> + (set ? CONFIG_VSC9953_VLAN_PORT_MASK : 0));
>
> It may also help to rename the parameter from "set" to something like
> "set_member".
Ok.
> > + /* Administrative down */
> > + if ((!vsc9953_l2sw.port[port_no].enabled)) {
>
> Why do you have double "((" and "))"?
By mistake. I will remove the extra pair.
> > + int i;
>
> Use a single space.
Ok, I will make sure there are no tabs after a variable's type, in all these patches.
> > + 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.
> > +#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 could also use those from bitfield.h, but then I
should create new macros or use some sort of magic numbers for the
"shift" parameter. Changing the already defined bitfiled_*() functions
doesn't look like an option since it would require changes all over
u-boot.
> > +struct vsc9953_rew_port {
> > + u32 port_vlan_cfg;
> > + u32 port_tag_cfg;
> > + u32 port_port_cfg;
> > + u32 port_dscp_cfg;
> > + u32 port_pcp_dei_qos_map_cfg[16];
>
> Seems like you should drop the "port_" from all of these member names.
Yes, I will remove the "port_".
> > +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.
Thanks and best regards,
Codrin
More information about the U-Boot
mailing list