Hi Codrin,
On Wed, Jul 1, 2015 at 9:31 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@freescale.com wrote:
Hi Joe,
-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@gmail.com] Sent: Wednesday, July 01, 2015 1:26 AM To: Ciubotariu Codrin Constantin-B43658 Cc: u-boot; Joe Hershberger; Sun York-R58495 Subject: Re: [U-Boot] [PATCH 03/11 v2] drivers/net/vsc9953: Add default configuration for VSC9953 L2 Switch
Hi Codrin,
On Tue, Jun 30, 2015 at 2:51 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@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.
Yes, VSC9953_TAG_CFG_ALL_BUT_PVID_ZERO and VSC9953_TAG_CFG_ALL_BUT_ZERO.
Sounds good.
> +#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;+}
Ok, should we assure we return 0 if mask is 0? Something like return mask : ffs(mask) - 1 ? 0;
Sounds like a good idea.
/* 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);+}
Ok.
Best regards, Codrin
Cheers, -Joe