From: Alexander Couzens <[email protected]>
Implement mtk_pcs_ops for the SGMII pcs to read the current state
of the hardware.
Signed-off-by: Alexander Couzens <[email protected]>
Signed-off-by: Frank Wunderlich <[email protected]>
Fixes: 14a44ab0330d ("net: mtk_eth_soc: partially convert to phylink_pcs")
---
without this Patch i get this trace in 6.1-rc1 when enabling the left
sfp (eth1) on bpi-r3 (crash happens in phylink core because a NULL-pointer,
so fixes-Tag is a guess):
[ 108.302810] Unable to handle kernel execute from non-executable memory at virtual address 0000000000000000
[ 108.312462] Mem abort info:
[ 108.315263] ESR = 0x0000000086000005
[ 108.319003] EC = 0x21: IABT (current EL), IL = 32 bits
[ 108.324335] SET = 0, FnV = 0
[ 108.327378] EA = 0, S1PTW = 0
[ 108.330505] FSC = 0x05: level 1 translation fault
[ 108.335375] user pgtable: 4k pages, 39-bit VAs, pgdp=00000000419be000
[ 108.341798] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000, pu
d=0000000000000000
[ 108.350489] Internal error: Oops: 0000000086000005 [#1] SMP
[ 108.356047] Modules linked in: cdc_mbim cdc_ncm cdc_wdm cdc_ether qcserial us
a_wwan usbnet usbser
a Mmeisis
ge from syslogd@bpi-r3 at Aug 7 15:26:54 ...
kernel:[ 108.350489] Internal error: Oops: 0000000086000005 [#1] SMP
[ 108.376743] CPU: 3 PID: 8 Comm: kworker/u8:0 Not tainted 6.0.0-bpi-r3 #1
[ 108.383461] Hardware name: Bananapi BPI-R3 (sdmmc) (DT)
[ 108.388671] Workqueue: events_power_efficient phylink_resolve
[ 108.394411] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 108.401355] pc : 0x0
[ 108.403531] lr : phylink_mac_pcs_get_state+0x7c/0x100
[ 108.408572] sp : ffffffc00963bd00
[ 108.411873] x29: ffffffc00963bd00 x28: 0000000000000000 x27: 0000000000000000
[ 108.418994] x26: ffffff80000ed074 x25: ffffff800001c105 x24: 0000000000000000
[ 108.426116] x23: ffffff80022f8cd8 x22: ffffff80022f8d30 x21: ffffff80022f8c00
[ 108.433236] x20: 0000000000000000 x19: ffffff8000126040 x18: 0000000000000000
[ 108.440357] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[ 108.447478] x14: ffffffffffffffff x13: 0000000000000030 x12: 0101010101010101
[ 108.454598] x11: 7f7f7f7f7f7f7f7f x10: fefefefefefefeff x9 : ffffff80000ed07c
[ 108.461720] x8 : fefefefefefefeff x7 : 0000000000000017 x6 : ffffff80000ed074
[ 108.468840] x5 : 0000000000000000 x4 : 0000020000006440 x3 : 00000000fffffffa
[ 108.475960] x2 : 0000000000000000 x1 : ffffffc00963bd80 x0 : ffffff80014c3ab0
[ 108.483082] Call trace:
[ 108.485516] 0x0
[ 108.487344] phylink_resolve+0x248/0x520
[ 108.491256] process_one_work+0x204/0x478
[ 108.495256] worker_thread+0x148/0x4c0
[ 108.498993] kthread+0xdc/0xe8
[ 108.502037] ret_from_fork+0x10/0x20
[ 108.505608] Code: bad PC value
[ 108.508652] ---[ end trace 0000000000000000 ]---
[ 108.513255] Kernel panic - not syncing: Oops: Fatal exception
[ 108.518984] SMP: stopping secondary CPUs
[ 108.522894] Kernel Offset: disabled
[ 108.526369] CPU features: 0x00000,00800084,0000420b
[ 108.531232] Memory Limit: none
[ 108.534274] Rebooting in 1 seconds..
---
drivers/net/ethernet/mediatek/mtk_sgmii.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
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)
--
2.34.1
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Am 18. Oktober 2022 18:39:01 MESZ schrieb "Russell King (Oracle)" <[email protected]>:
>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)" <[email protected]>:
> >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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> Gesendet: Donnerstag, 20. Oktober 2022 um 10:33 Uhr
> Von: "Russell King (Oracle)" <[email protected]>
> An: "Frank Wunderlich" <[email protected]>
> Cc: "Frank Wunderlich" <[email protected]>, [email protected], "Alexander Couzens" <[email protected]>, "Felix Fietkau" <[email protected]>, "John Crispin" <[email protected]>, "Sean Wang" <[email protected]>, "Mark Lee" <[email protected]>, "David S. Miller" <[email protected]>, "Eric Dumazet" <[email protected]>, "Jakub Kicinski" <[email protected]>, "Paolo Abeni" <[email protected]>, "Matthias Brugger" <[email protected]>, [email protected], [email protected], [email protected]
> 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)" <[email protected]>:
> > >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