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));elseclrsetbits_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