[U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch
Joe Hershberger
joe.hershberger at gmail.com
Mon Jun 29 22:30:09 CEST 2015
Hi Codrin,
On Mon, Jun 29, 2015 at 11:06 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu at freescale.com> wrote:
> 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.
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.
>> > +#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.
> 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.
That could be done, but if you do create macros for the shift
parameter, they should go in bitfield.h.
Please do not add magic numbers.
> Changing the already defined bitfiled_*() functions
> doesn't look like an option since it would require changes all over
> u-boot.
Yes, certainly do not do this. Use them as is or add new versions. Do
no change the existing functions.
>> > +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.
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.
Thanks,
-Joe
More information about the U-Boot
mailing list