netdev - Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 8 Dec 2015 06:30:56 -0800
From:	Tim Harvey <tharvey@...eworks.com>
To:	Krzysztof Hałasa <khalasa@...p.pl>
Cc:	netdev <netdev@...r.kernel.org>, Shawn Guo <shawnguo@...nel.org>,
	Fabio Estevam <Fabio.Estevam@...escale.com>
Subject: Re: [PATCH] I.MX6: Fix Ethernet PHY mode on Ventana boards

On Tue, Dec 8, 2015 at 6:13 AM, Krzysztof Hałasa <khalasa@...p.pl> wrote:
> Tim Harvey <tharvey@...eworks.com> writes:
>
>> It sounds like your saying this controls whether the phy is in charge
>> of delay vs the MAC. I have never needed to set this and haven't found
>> where its actually used (in at least 4.3). Is this caused by something
>> new in the kernel I haven't seen yet or is it possible you have board
>> that has an Ethernet issue?
>
> I think you aren't using the Marvell PHY driver :-)
> The existing DTS files work fine with the "default" PHY driver.
> It's only the Marvell driver (drivers/net/phy/marvell.c, aka
> CONFIG_MARVELL_PHY) which have the issue.
>

Kryzsztof,

True - I have not used CONFIG_MARVELL_PHY and didn't know of its existance.

> The problem is caused by the following code in m88e1121_config_aneg():
>
>         if (phy_interface_is_rgmii(phydev)) {
>
>                 mscr = phy_read(phydev, MII_88E1121_PHY_MSCR_REG) &
>                         MII_88E1121_PHY_MSCR_DELAY_MASK;
>
>                 if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>                         mscr |= (MII_88E1121_PHY_MSCR_RX_DELAY |
>                                  MII_88E1121_PHY_MSCR_TX_DELAY);
>                 else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
>                         mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
>                 else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>                         mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
>
>                 err = phy_write(phydev, MII_88E1121_PHY_MSCR_REG, mscr);
>                 if (err < 0)
>                         return err;
>         }
>
> mscr initially holds the correct value (one with RX_DELAY and TX_DELAY
> bits set). Since the _*ID are not specified, these bits are reset and
> (now incorrect) value is written back to the register.
> OTOH the generic PHY driver knows nothing about the MSCR and thus
> doesn't "corrupt" it.
>
> The whole -ID thing is about signal trace lengths (on PCB), where the
> non-ID setup requires special, longer traces for certain signals to
> compensate for propagation times etc. The -ID version does it internally
> (when enabled) and the PCB layout can be a bit simpler.

right - I understand what the delay lines are, it just struck me as
strange that I've never needed this.

>
> To be honest I only checked it on a single GW5400 rev. C, but I'd expect
> it to be exactly the same across all GW5[234]*. Can test on GW5[23] if
> there is any doubt (in few days I guess).
>

no doubt now. I re-created your config and see the problem as well.

The marvell phy driver (drivers/net/phy/marvell.c) has a very lacking
Kconfig stating its a PHY driver for the 88E1011S when in fact it has
id's for 12 ID's including the 88E1510 we use on the GW51xx, GW52xx,
GW53xx, GW54xx.

This may be a stupid question but what does the Marvell phy driver
offer that the generic phy driver does not?

Thanks for finding this and submitting a patch!

Acked-by: Tim Harvey <tharvey@...eworks.com>

Tim
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html