| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Mar 2019 20:37:09 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: liweihang <liweihang@...ilicon.com>,
Phil Reid <preid@...ctromag.com.au>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc: Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
"dongsheng.wang@...-semitech.com" <dongsheng.wang@...-semitech.com>,
"cphealy@...il.com" <cphealy@...il.com>,
"clemens.gruber@...ruber.com" <clemens.gruber@...ruber.com>,
"hkallweit1@...il.com" <hkallweit1@...il.com>,
"nbd@....name" <nbd@....name>,
"harini.katakam@...inx.com" <harini.katakam@...inx.com>
Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
On 3/19/2019 7:34 PM, liweihang wrote:
> Hi all,
>
> I've met a similar issue and sent an email to discuss about it before:
> Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy
>
> d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
> I reverted this patch and the auto-negotiation works ok.
>
> Florian, could you please read my previous email and give me some advice?
If you can copy the patch author on that email the next time that will
help expedite things.
So the problem seems to come from the fact that unless the BCMR_RESET
bit is written, then m88e1121_config_aneg_rgmii_delays() has no effect,
does that sound like what you are observing?
Does the following work for you (Phil and yourself)?
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3ccba37bd6dd..6a1ea4c2042a 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -448,6 +448,10 @@ static int m88e1121_config_aneg(struct phy_device
*phydev)
err = m88e1121_config_aneg_rgmii_delays(phydev);
if (err < 0)
return err;
+
+ err = genphy_soft_reset(phydev);
+ if (err < 0)
+ return err;
}
err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>
>
>> -----Original Message-----
>> From: netdev-owner@...r.kernel.org [mailto:netdev-
>> owner@...r.kernel.org] On Behalf Of Phil Reid
>> Sent: Wednesday, March 20, 2019 9:34 AM
>> To: Florian Fainelli <f.fainelli@...il.com>; netdev@...r.kernel.org
>> Cc: Andrew Lunn <andrew@...n.ch>; David S. Miller
>> <davem@...emloft.net>; dongsheng.wang@...-semitech.com;
>> cphealy@...il.com; clemens.gruber@...ruber.com; hkallweit1@...il.com;
>> nbd@....name; harini.katakam@...inx.com
>> Subject: Re: regression from: net: phy: marvell: Avoid unnecessary soft reset
>>
>> On 20/03/2019 12:53 am, Florian Fainelli wrote:
>>> On 3/18/19 6:32 PM, Phil Reid wrote:
>>>> On 19/03/2019 1:09 am, Florian Fainelli wrote:
>>>>> On 3/17/19 7:11 PM, Phil Reid wrote:
>>>>>> On 16/03/2019 5:58 am, Florian Fainelli wrote:
>>>>>>> On 3/15/19 1:52 AM, Phil Reid wrote:
>>>>>>>> G'day All,
>>>>>>>>
>>>>>>>> I've just update from kernel 4.19 to 5.0 on a custom board that
>>>>>>>> has a marvell dsa mv88e6085 and the phy on the mv88e6085 will
>>>>>>>> only connect at 10Mb/s with the above mentioned patch applied.
>>>>>>>>
>>>>>>>> Bisecting the issue lead me to the following patch.
>>>>>>>>
>>>>>>>> d6ab93364734bd (net: phy: marvell: Avoid unnecessary soft reset)
>>>>>>>>
>>>>>>>> Revert the patch, and the associated build fix
>>>>>>>> 4b1bd6976945417 (net: phy: marvell: Fix build.) restores
>>>>>>>> connections to 1Gb/s.
>>>>>>>>
>>>>>>>> Anyone have any thoughts as to the correct fix?
>>>>>>>
>>>>>>> What is the PHY OUI (MII_PHYSID1/ID2) for that PHY? We may need
>> to
>>>>>>> add a specific entry in drivers/net/phy/marvell.c to restore the
>>>>>>> software reset to commit changes to the register.
>>>>>>>
>>>>>> G'day Florian,
>>>>>>
>>>>>> OUI is 0x005043
>>>>>> Model is 101011
>>>>>>
>>>>>> Phy1ID: 0x0141
>>>>>> Phy2ID: 0x0eb1
>>>>>>
>>>>>> The running phy driver is "Marvell 88E1540"
>>>>>
>>>>> Thanks, you mentioned a mv88e6085 but that chip is a 10/100 switch,
>>>>> did you mean that the mv88e6085 compatible string is used in Device
>>>>> Tree to designate that chip? While the 88E1540 driver is picked up,
>>>>> that driver is originally for external PHY packages (AFAICT), so
>>>>> there could be some switch-specific integration of this PHY that
>>>>> makes it behave differently, having the OUI helps narrow down what
>>>>> might be necessary to accomplish.
>>>>>
>>>>> FWIW, the changes were originally tested with a 88e6352.
>>>>>
>>>>
>>>> Oops yes that's the compatible string.
>>>> Mfr Code we purchased was 88E6352-A1-TFJ2I000
>>>>
>>>> The switch id registers (0x03) returns 0x3521
>>>>
>>>> Odd it works ok for others.
>>>
>>> One possible difference is that the device this was tested with has an
>>> EEPROM which gets involved in configuring the switch during PoR (IIRC)
>>> and that could produce different results. Do you have any switch
>>> initialization happening in the boot loader by any chance?
>>>
>> Yes uboot is configuring the switch at boot as a simple network switch.
>> Then we reconfig it in the kernel. I see if I can get some time to bypass the
>> uboot init and just rely on the kernel to config the chip.
>>
>> The external reset pin is available as well. I tried adding that to the dts but
>> that seems to make things worse. With the patch reverted and external
>> reset pin enabled I see a message saying that the phy connected at 1G but
>> can not receive anything, even on the fixed phy link to our wifi device.
>>
>> This is the config being applied by uboot if it helps.
>>
>> /* ------------------------------------------------------------------------- */ struct
>> mv88e_sw_reg extsw_conf[] = {
>> /*
>> * port 0, PIGGY4, autoneg
>> * first the fix for the 1000Mbits Autoneg, this is from
>> * a Marvell errata, the regs are undocumented
>> */
>> { PHY(0), PHY_PAGE, AN1000FIX_PAGE },
>> { PHY(0), PHY_STATUS, AN1000FIX },
>> { PHY(0), PHY_PAGE, 0 },
>> /* now the real port and phy configuration */
>> /* port 0 unused */
>> { PORT(0), PORT_CTRL, PORT_DIS },
>> { PHY (0), PHY_PAGE, 0 },
>> { PHY (0), PHY_CTRL, PHY_PWR_DOWN },
>> { PHY (0), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
>> /* port 1 unused */
>> { PORT(1), PORT_CTRL, PORT_DIS },
>> { PHY (1), PHY_PAGE, 0 },
>> { PHY (1), PHY_CTRL, PHY_PWR_DOWN },
>> { PHY (1), PHY_SPEC_CTRL, SPEC_PWR_DOWN },
>> /* port 2, (physical jack 0) */
>> { PORT(2), PORT_PHY, NO_SPEED_FOR },
>> { PORT(2), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> { PHY (2), PHY_PAGE, 0 },
>> { PHY (2), PHY_1000_CTRL, ADV_1000_FDPX },
>> { PHY (2), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> { PHY (2), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> /* port 3, (physical jack 1) */
>> { PORT(3), PORT_PHY, NO_SPEED_FOR },
>> { PORT(3), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> { PHY (3), PHY_PAGE, 0 },
>> { PHY (3), PHY_1000_CTRL, ADV_1000_FDPX },
>> { PHY (3), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> { PHY (3), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> /* port 4, (physical jack 2) */
>> { PORT(4), PORT_PHY, NO_SPEED_FOR },
>> { PORT(4), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> { PHY (4), PHY_PAGE, 0 },
>> { PHY (4), PHY_1000_CTRL, ADV_1000_FDPX },
>> { PHY (4), PHY_SPEC_CTRL, AUTO_MDIX_EN },
>> { PHY (4), PHY_CTRL, PHY_SW_RESET | PHY_1_GBPS | AUTONEG_EN
>> | AUTONEG_RST | FULL_DUPLEX },
>> /* port 5, CPU_RGMII */
>> //{ PORT(5), PORT_CTRL, PORT_DIS },
>> { PORT(5), PORT_PHY, FLOW_CTRL_EN |
>> FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX | FULL_DPX_FOR |
>> SPEED_100_FOR },
>> { PORT(5), PORT_CTRL, FORWARDING | EGRS_FLD_ALL },
>> { PORT(5), PORT_LED, PORT_LED_UPDATE | PORT_LED_CTL |
>> PORT5_L0_ACT5 | PORT5_L1_ACT6 },
>> /* port 6, unused, this port has no phy */
>> { PORT(6), PORT_PHY, RX_RGMII_TIM | TX_RGMII_TIM |
>> FLOW_CTRL_EN | FLOW_CTRL_FOR | LINK_VAL | LINK_FOR | FULL_DPX |
>> FULL_DPX_FOR | SPEED_1000_FOR },
>> { PORT(6), PORT_CTRL, FORWARDING | EGRS_FLD_ALL }, };
>>
>>>>
>>>> Here's the relevant dmesgs (with the patch reverted)
>>>>
>>>> [ 1.332652] bus: 'mdio_bus': add driver mv88e6085 [ 1.368572]
>>>> bus: 'mdio_bus': driver_probe_device: matched device
>>>> stmmac-0:00 with driver mv88e6085
>>>> [ 1.368584] bus: 'mdio_bus': really_probe: probing driver
>>>> mv88e6085 with device stmmac-0:00 [ 1.368603] mv88e6085
>>>> stmmac-0:00: no pinctrl handle [ 1.368630] mv88e6085 stmmac-0:00:
>>>> GPIO lookup for consumer reset [ 1.368638] mv88e6085 stmmac-0:00:
>>>> using device tree for GPIO lookup [ 1.368687] mv88e6085
>>>> stmmac-0:00: using lookup tables for GPIO lookup [ 1.368696]
>>>> mv88e6085 stmmac-0:00: No GPIO consumer reset found [ 1.368835]
>>>> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
>>>> revision 1 [ 1.401845] mdio_bus stmmac-0:00: Driver mv88e6085
>>>> requests probe deferral [ 2.497476] bus: 'mdio_bus':
>>>> driver_probe_device: matched device
>>>> stmmac-0:00 with driver mv88e6085
>>>> [ 2.497486] bus: 'mdio_bus': really_probe: probing driver
>>>> mv88e6085 with device stmmac-0:00 [ 2.497505] mv88e6085
>>>> stmmac-0:00: no pinctrl handle [ 2.497536] mv88e6085 stmmac-0:00:
>>>> GPIO lookup for consumer reset [ 2.497544] mv88e6085 stmmac-0:00:
>>>> using device tree for GPIO lookup [ 2.497595] mv88e6085
>>>> stmmac-0:00: using lookup tables for GPIO lookup [ 2.497604]
>>>> mv88e6085 stmmac-0:00: No GPIO consumer reset found [ 2.497750]
>>>> mv88e6085 stmmac-0:00: switch 0x3520 detected: Marvell 88E6352,
>>>> revision 1 [ 2.862241] mv88e6085 stmmac-0:00 lan1 (uninitialized):
>>>> PHY [!soc!ethernet@...02000!mdio!switch0@...dio:02] driver [Marvell
>>>> 88E1540] [ 2.884968] mv88e6085 stmmac-0:00 lan2 (uninitialized):
>>>> PHY [!soc!ethernet@...02000!mdio!switch0@...dio:03] driver [Marvell
>>>> 88E1540] [ 2.905318] mv88e6085 stmmac-0:00 lan3 (uninitialized):
>>>> PHY [!soc!ethernet@...02000!mdio!switch0@...dio:04] driver [Marvell
>>>> 88E1540] [ 2.917034] driver: 'mv88e6085': driver_bound: bound to
>>>> device 'stmmac-0:00'
>>>> [ 2.917127] bus: 'mdio_bus': really_probe: bound device
>>>> stmmac-0:00 to driver mv88e6085 [ 8.293526] mv88e6085 stmmac-0:00
>>>> lan3: configuring for phy/ link mode [ 8.305856] mv88e6085
>>>> stmmac-0:00 lan2: configuring for phy/ link mode [ 8.325009]
>>>> mv88e6085 stmmac-0:00 lan1: configuring for phy/ link mode [
>>>> 8.342635] mv88e6085 stmmac-0:00 wifi: configuring for fixed/ link
>>>> mode [ 8.358912] mv88e6085 stmmac-0:00 wifi: Link is Up -
>>>> 100Mbps/Full - flow control off [ 12.034974] mv88e6085 stmmac-0:00
>>>> lan1: Link is Up - 1Gbps/Full - flow control rx/tx
>>>>
>>>>
>>>
>>>
>>
>>
>> --
>> Regards
>> Phil
--
Florian
Powered by blists - more mailing lists