| Message ID | 20171010124953.386-2-privat@egil-hjelmeland.no |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | lan9303: Add basic offloading of unicast traffic | expand |
> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */ > +static int lan9303_setup_tagging(struct lan9303 *chip) > +{ > + int ret; > + u32 val; > + /* enable defining the destination port via special VLAN tagging > + * for port 0 > + */ > + ret = lan9303_write_switch_reg(chip, > LAN9303_SWE_INGRESS_PORT_TYPE, > + > LAN9303_SWE_INGRESS_PORT_TYPE_VLAN); > + if (ret) > + return ret; > + > + /* tag incoming packets at port 1 and 2 on their way to port 0 to be > + * able to discover their source port > + */ > + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0; > + return lan9303_write_switch_reg(chip, > LAN9303_BM_EGRSS_PORT_TYPE, val); Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 like previous line? > @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) > return -EINVAL; > } > > + ret = lan9303_setup_tagging(chip); > + if (ret) > + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); > + Still move on when error happens? > ret = lan9303_separate_ports(chip); > if (ret) > dev_err(chip->dev, "failed to separate ports %d\n", ret); > -- > 2.11.0 - Woojung
On 10. okt. 2017 17:14, Woojung.Huh@microchip.com wrote: >> +/* forward special tagged packets from port 0 to port 1 *or* port 2 */ >> +static int lan9303_setup_tagging(struct lan9303 *chip) >> +{ >> + int ret; >> + u32 val; >> + /* enable defining the destination port via special VLAN tagging >> + * for port 0 >> + */ >> + ret = lan9303_write_switch_reg(chip, >> LAN9303_SWE_INGRESS_PORT_TYPE, >> + >> LAN9303_SWE_INGRESS_PORT_TYPE_VLAN); >> + if (ret) >> + return ret; >> + >> + /* tag incoming packets at port 1 and 2 on their way to port 0 to be >> + * able to discover their source port >> + */ >> + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0; >> + return lan9303_write_switch_reg(chip, >> LAN9303_BM_EGRSS_PORT_TYPE, val); > Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 > like previous line? > Specific reason was to please a reviewer that did not like my indenting in first version. I did not agree with him, but since nobody else spoke up, I changed the code. >> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) >> return -EINVAL; >> } >> >> + ret = lan9303_setup_tagging(chip); >> + if (ret) >> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); >> + > Still move on when error happens? > Good question. I just followed the pattern from the original function, which was not made by me. Actually I did once reflect on whether this was the correct way. Perhaps it could be argued that it is better to allow the device to come up, so the problem can be investigated? >> ret = lan9303_separate_ports(chip); >> if (ret) >> dev_err(chip->dev, "failed to separate ports %d\n", ret); >> -- >> 2.11.0 > > - Woojung >
> > Specific reason to use val then using > LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 > > like previous line? > > > Specific reason was to please a reviewer that did not like my > indenting in first version. I did not agree with him, but since > nobody else spoke up, I changed the code. Got it. Missed previous patch/comment. > >> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) > >> return -EINVAL; > >> } > >> > >> + ret = lan9303_setup_tagging(chip); > >> + if (ret) > >> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); > >> + > > Still move on when error happens? > > > Good question. I just followed the pattern from the original function, > which was not made by me. Actually I did once reflect on whether this > was the correct way. Perhaps it could be argued that it is better to > allow the device to come up, so the problem can be investigated? Maybe depends on severity of setting? BTW, lan9303_setup() still returns ZERO at the end? Thanks. Woojung
Hi Egil, Egil Hjelmeland <privat@egil-hjelmeland.no> writes: >>> + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0; >>> + return lan9303_write_switch_reg(chip, >>> LAN9303_BM_EGRSS_PORT_TYPE, val); >> Specific reason to use val then using LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 >> like previous line? >> > Specific reason was to please a reviewer that did not like my > indenting in first version. I did not agree with him, but since > nobody else spoke up, I changed the code. Your indentation was broken and did not respect the Kernel Coding Style. Using a temporary variable here ensures you respect both the 80-char limit and the vertical alignment on opening parenthesis. You'd like to use ./scripts/checkpatch.pl before submitting patches. Thanks, Vivien
On 10. okt. 2017 17:51, Woojung.Huh@microchip.com wrote: >>> Specific reason to use val then using >> LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 >>> like previous line? >>> >> Specific reason was to please a reviewer that did not like my >> indenting in first version. I did not agree with him, but since >> nobody else spoke up, I changed the code. > Got it. Missed previous patch/comment. > >>>> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) >>>> return -EINVAL; >>>> } >>>> >>>> + ret = lan9303_setup_tagging(chip); >>>> + if (ret) >>>> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); >>>> + >>> Still move on when error happens? >>> >> Good question. I just followed the pattern from the original function, >> which was not made by me. Actually I did once reflect on whether this >> was the correct way. Perhaps it could be argued that it is better to >> allow the device to come up, so the problem can be investigated? > Maybe depends on severity of setting? > BTW, lan9303_setup() still returns ZERO at the end? I did quick survey of the _setup functions of the other dsa drivers. Some return on error, some ignore errors. If you think so, I can make a v3 series that return on error. Otherwise I leave it as it is. > > Thanks. > Woojung > Thank you for polite review. Egil
> >>>> @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) > >>>> return -EINVAL; > >>>> } > >>>> > >>>> + ret = lan9303_setup_tagging(chip); > >>>> + if (ret) > >>>> + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); > >>>> + > >>> Still move on when error happens? > >>> > >> Good question. I just followed the pattern from the original function, > >> which was not made by me. Actually I did once reflect on whether this > >> was the correct way. Perhaps it could be argued that it is better to > >> allow the device to come up, so the problem can be investigated? > > Maybe depends on severity of setting? > > BTW, lan9303_setup() still returns ZERO at the end? > I did quick survey of the _setup functions of the other dsa drivers. > Some return on error, some ignore errors. > If you think so, I can make a v3 series that return on error. Otherwise > I leave it as it is. Unless Andrew, Vivien or Florian raises flag, I guess it will be fine as-is. Thanks. Woojung
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index 07355db2ad81..2215ec1fbe1e 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -157,6 +157,7 @@ # define LAN9303_SWE_PORT_MIRROR_ENABLE_RX_MIRRORING BIT(1) # define LAN9303_SWE_PORT_MIRROR_ENABLE_TX_MIRRORING BIT(0) #define LAN9303_SWE_INGRESS_PORT_TYPE 0x1847 +#define LAN9303_SWE_INGRESS_PORT_TYPE_VLAN 3 #define LAN9303_BM_CFG 0x1c00 #define LAN9303_BM_EGRSS_PORT_TYPE 0x1c0c # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT2 (BIT(17) | BIT(16)) @@ -510,11 +511,30 @@ static int lan9303_enable_processing_port(struct lan9303 *chip, LAN9303_MAC_TX_CFG_X_TX_ENABLE); } +/* forward special tagged packets from port 0 to port 1 *or* port 2 */ +static int lan9303_setup_tagging(struct lan9303 *chip) +{ + int ret; + u32 val; + /* enable defining the destination port via special VLAN tagging + * for port 0 + */ + ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE, + LAN9303_SWE_INGRESS_PORT_TYPE_VLAN); + if (ret) + return ret; + + /* tag incoming packets at port 1 and 2 on their way to port 0 to be + * able to discover their source port + */ + val = LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0; + return lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE, val); +} + /* We want a special working switch: * - do not forward packets between port 1 and 2 * - forward everything from port 1 to port 0 * - forward everything from port 2 to port 0 - * - forward special tagged packets from port 0 to port 1 *or* port 2 */ static int lan9303_separate_ports(struct lan9303 *chip) { @@ -529,22 +549,6 @@ static int lan9303_separate_ports(struct lan9303 *chip) if (ret) return ret; - /* enable defining the destination port via special VLAN tagging - * for port 0 - */ - ret = lan9303_write_switch_reg(chip, LAN9303_SWE_INGRESS_PORT_TYPE, - 0x03); - if (ret) - return ret; - - /* tag incoming packets at port 1 and 2 on their way to port 0 to be - * able to discover their source port - */ - ret = lan9303_write_switch_reg(chip, LAN9303_BM_EGRSS_PORT_TYPE, - LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0); - if (ret) - return ret; - /* prevent port 1 and 2 from forwarding packets by their own */ return lan9303_write_switch_reg(chip, LAN9303_SWE_PORT_STATE, LAN9303_SWE_PORT_STATE_FORWARDING_PORT0 | @@ -644,6 +648,10 @@ static int lan9303_setup(struct dsa_switch *ds) return -EINVAL; } + ret = lan9303_setup_tagging(chip); + if (ret) + dev_err(chip->dev, "failed to setup port tagging %d\n", ret); + ret = lan9303_separate_ports(chip); if (ret) dev_err(chip->dev, "failed to separate ports %d\n", ret);
Prepare for next patch: Move tag setup from lan9303_separate_ports() to new function lan9303_setup_tagging() Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no> --- drivers/net/dsa/lan9303-core.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)