| Message ID | 20221018153506.60944-1-linux@fw-web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mtk_sgmii: implement mtk_pcs_ops | expand |
Hi, A couple of points: On Tue, Oct 18, 2022 at 05:35:06PM +0200, Frank Wunderlich wrote: > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c > index 736839c84130..9614973fd9c4 100644 > --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c > +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c > @@ -122,10 +122,25 @@ static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, > regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); > } > > +static void mtk_pcs_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state) > +{ > + struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs); > + unsigned int val; > + > + regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); > + state->speed = val & RG_PHY_SPEED_3_125G ? SPEED_2500 : SPEED_1000; > + > + regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); > + state->an_complete = !!(val & SGMII_AN_COMPLETE); > + state->link = !!(val & SGMII_LINK_STATYS); > + state->pause = 0; Finally, something approaching a reasonable implementation for this! Two points however: 1) There's no need to set state->pause if there is no way to get that state. 2) There should also be a setting for state->pause. > +} > + > static const struct phylink_pcs_ops mtk_pcs_ops = { > .pcs_config = mtk_pcs_config, > .pcs_an_restart = mtk_pcs_restart_an, > .pcs_link_up = mtk_pcs_link_up, > + .pcs_get_state = mtk_pcs_get_state, Please keep this in order - pcs_get_state should be just before pcs_config. Thanks.
Am 18. Oktober 2022 18:39:01 MESZ schrieb "Russell King (Oracle)" <linux@armlinux.org.uk>: >Hi, > >A couple of points: > >On Tue, Oct 18, 2022 at 05:35:06PM +0200, Frank Wunderlich wrote: >> + regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); >> + state->an_complete = !!(val & SGMII_AN_COMPLETE); >> + state->link = !!(val & SGMII_LINK_STATYS); >> + state->pause = 0; > >Finally, something approaching a reasonable implementation for this! >Two points however: >1) There's no need to set state->pause if there is no way to get that > state. >2) There should also be a setting for state->pause. Currently it looks like pause cannot be controlled in sgmii-mode so we disabled it here to not leave it undefined. Should i drop assignment here? >> +} >> + >> static const struct phylink_pcs_ops mtk_pcs_ops = { >> .pcs_config = mtk_pcs_config, >> .pcs_an_restart = mtk_pcs_restart_an, >> .pcs_link_up = mtk_pcs_link_up, >> + .pcs_get_state = mtk_pcs_get_state, > >Please keep this in order - pcs_get_state should be just before >pcs_config. Ok,will change order regards Frank
On Thu, Oct 20, 2022 at 07:54:49AM +0200, Frank Wunderlich wrote: > Am 18. Oktober 2022 18:39:01 MESZ schrieb "Russell King (Oracle)" <linux@armlinux.org.uk>: > >Hi, > > > >A couple of points: > > > >On Tue, Oct 18, 2022 at 05:35:06PM +0200, Frank Wunderlich wrote: > > >> + regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); > >> + state->an_complete = !!(val & SGMII_AN_COMPLETE); > >> + state->link = !!(val & SGMII_LINK_STATYS); > >> + state->pause = 0; > > > >Finally, something approaching a reasonable implementation for this! > >Two points however: > >1) There's no need to set state->pause if there is no way to get that > > state. > >2) There should also be a setting for state->pause. > > Currently it looks like pause cannot be controlled in sgmii-mode so we disabled it here to not leave it undefined. Should i drop assignment here? Why do you think it would be undefined? static void phylink_mac_pcs_get_state(struct phylink *pl, struct phylink_link_state *state) { ... if (state->an_enabled) { ... state->pause = MLO_PAUSE_NONE; } else { ,,, state->pause = pl->link_config.pause; } ... if (pl->pcs) pl->pcs->ops->pcs_get_state(pl->pcs, state); So, phylink will call your pcs_get_state() function having initialised it to something sensible.
> Gesendet: Donnerstag, 20. Oktober 2022 um 10:33 Uhr > Von: "Russell King (Oracle)" <linux@armlinux.org.uk> > An: "Frank Wunderlich" <frank-w@public-files.de> > Cc: "Frank Wunderlich" <linux@fw-web.de>, linux-mediatek@lists.infradead.org, "Alexander Couzens" <lynxis@fe80.eu>, "Felix Fietkau" <nbd@nbd.name>, "John Crispin" <john@phrozen.org>, "Sean Wang" <sean.wang@mediatek.com>, "Mark Lee" <Mark-MC.Lee@mediatek.com>, "David S. Miller" <davem@davemloft.net>, "Eric Dumazet" <edumazet@google.com>, "Jakub Kicinski" <kuba@kernel.org>, "Paolo Abeni" <pabeni@redhat.com>, "Matthias Brugger" <matthias.bgg@gmail.com>, netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org > Betreff: Re: [PATCH] net: mtk_sgmii: implement mtk_pcs_ops > > On Thu, Oct 20, 2022 at 07:54:49AM +0200, Frank Wunderlich wrote: > > Am 18. Oktober 2022 18:39:01 MESZ schrieb "Russell King (Oracle)" <linux@armlinux.org.uk>: > > >Hi, > > > > > >A couple of points: > > > > > >On Tue, Oct 18, 2022 at 05:35:06PM +0200, Frank Wunderlich wrote: > > > > >> + regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); > > >> + state->an_complete = !!(val & SGMII_AN_COMPLETE); > > >> + state->link = !!(val & SGMII_LINK_STATYS); > > >> + state->pause = 0; > > > > > >Finally, something approaching a reasonable implementation for this! > > >Two points however: > > >1) There's no need to set state->pause if there is no way to get that > > > state. > > >2) There should also be a setting for state->pause. > > > > Currently it looks like pause cannot be controlled in sgmii-mode so we disabled it here to not leave it undefined. Should i drop assignment here? > > Why do you think it would be undefined? > > static void phylink_mac_pcs_get_state(struct phylink *pl, > struct phylink_link_state *state) > { > ... > if (state->an_enabled) { > ... > state->pause = MLO_PAUSE_NONE; > } else { > ,,, > state->pause = pl->link_config.pause; > } > ... > if (pl->pcs) > pl->pcs->ops->pcs_get_state(pl->pcs, state); > > So, phylink will call your pcs_get_state() function having initialised > it to something sensible. ok, then i drop the pause setting for now regards Frank
diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c index 736839c84130..9614973fd9c4 100644 --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -122,10 +122,25 @@ static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val); } +static void mtk_pcs_get_state(struct phylink_pcs *pcs, struct phylink_link_state *state) +{ + struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs); + unsigned int val; + + regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val); + state->speed = val & RG_PHY_SPEED_3_125G ? SPEED_2500 : SPEED_1000; + + regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &val); + state->an_complete = !!(val & SGMII_AN_COMPLETE); + state->link = !!(val & SGMII_LINK_STATYS); + state->pause = 0; +} + static const struct phylink_pcs_ops mtk_pcs_ops = { .pcs_config = mtk_pcs_config, .pcs_an_restart = mtk_pcs_restart_an, .pcs_link_up = mtk_pcs_link_up, + .pcs_get_state = mtk_pcs_get_state, }; int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)