Messages in this thread |  | | Date | Tue, 31 Mar 2020 18:46:40 +0100 | From | Russell King - ARM Linux admin <> | Subject | Re: [PATCH v2] ARM: imx: allow to disable board specific PHY fixups |
| |
On Tue, Mar 31, 2020 at 07:16:59PM +0200, Oleksij Rempel wrote: > On Tue, Mar 31, 2020 at 06:03:00PM +0100, Russell King - ARM Linux admin wrote: > > On Tue, Mar 31, 2020 at 10:44:59AM +0200, David Jander wrote: > > > I have checked with the datasheet of the AR8035, and AFAICS, what the code > > > does is this: > > > > > > - Disable the SmartEEE feature of the phy. The comment in the code implies > > > that for some reason it doesn't work, but the reason itself is not given. > > > Anyway, disabling SmartEEE should IMHO opinion be controlled by a DT > > > setting. There is no reason to believe this problem is specific to the > > > i.MX6. Besides, it is a feature of the phy, so it seems logical to expose > > > that via the DT. Once that is done, it has no place here. > > > > > > - Set the external clock output to 125MHz. This is needed because the i.MX6 > > > needs a 125MHz reference clock input. But it is not a requirement to use > > > this output. It is perfectly fine and possible to design a board that uses > > > an external oscillator for this. It is also possible that an i.MX6 design > > > has such a phy connected to a MAC behind a switch or some other interface. > > > Independent of i.MX6 this setting can also be necessary for other hardware > > > designs, based on different SoC's. In summary, this is a feature of the > > > specific hardware design at hand, and has nothing to do with the i.MX6 > > > specifically. This should definitely be exposed through the DT and not be > > > here. > > > > > > - Enable TXC delay. To clarify, the RGMII specification version 1 specified > > > that the RXC and TXC traces should be routed long enough to introduce a > > > certain delay to the clock signal, or the delay should be introduced via > > > other means. In a later version of the spec, a provision was given for MAC > > > or PHY devices to generate this delay internally. The i.MX6 MAC interface > > > is unable to generate the required delay internally, so it has to be taken > > > care of either by the board layout, or by the PHY device. This is the > > > crucial point: The amount of delay set by the PHY delay register depends on > > > the board layout. It should NEVER be hard-coded in SoC setup code. The > > > correct way is to specify it in the DT. Needless to say that this too, > > > isn't i.MX6-specific. > > > > Let's say this is simple to do, shall we? > > > > So, if I disable the call to ar8031_phy_fixup() from ar8035_phy_fixup(), > > and add the following to the imx6qdl-sr-som.dtsi fragment: > > > > &fec { > > ... > > phy-handle = <&phy>; > > > > mdio { > > #address-cells = <1>; > > #size-cells = <0>; > > > > phy: ethernet-phy@0 { > > reg = <0>; > > qca,clk-out-frequency = <125000000>; > > }; > > }; > > }; > > > > Note that phy-mode is already RGMII-ID. This should work, right? > > > > The link still comes up, which is good, but the PHY registers for > > the clock output are wrong. > > > > MMD 3 register 0x8016 contains 0xb282, not 0xb29a which it has > > _with_ the quirk - and thus the above clock frequency stated in > > DT is not being selected. Forcing this register to the right > > value restores networking. > > > > Yes, the PHY driver is being used: > > > > Qualcomm Atheros AR8035 2188000.ethernet-1:00: attached PHY driver [Qualcomm Atheros AR8035] (mii_bus:phy_addr=2188000.ethernet-1:00, irq=POLL) > > > > So that's not the problem. > > > > Adding some debug shows that the phy_device that is being used is > > the correct one: > > > > Qualcomm Atheros AR8035 2188000.ethernet-1:00: node=/soc/aips-bus@2100000/ethernet@2188000/mdio/ethernet-phy@0 > > > > and it is correctly parsing the clk-out-frequency property: > > > > Qualcomm Atheros AR8035 2188000.ethernet-1:00: cof=0 125000000 > > > > When we get to attaching the PHY however: > > > > Qualcomm Atheros AR8035 2188000.ethernet-1:00: clk_25m_mask=0004 clk_25m_reg=0000 > > > > which is just wrong. That's because: > > > > if (at803x_match_phy_id(phydev, ATH8030_PHY_ID) || > > at803x_match_phy_id(phydev, ATH8035_PHY_ID)) { > > priv->clk_25m_reg &= ~AT8035_CLK_OUT_MASK; > > priv->clk_25m_mask &= ~AT8035_CLK_OUT_MASK; > > } > > > > is patently untested - those "~" should not be there. These masks > > are one-bits-set for the values that comprise the fields, not > > zero-bits-set. > > > > So, I see a patch series is going to be necessary to fix the cockup(s) > > in the PHY driver before we can do anything with DT files. > > I'm glad you found this issues :D I made a patch to fix it last week. > And it was a reason to send a patch for disabling _all_ fixups :)
So I'm wasting my time creating a patch right now, and this patch to fix an obvious problem has not been picked up into -net yet, and isn't part of the 5.6 kernel.
Okay, I'll look a this again once 5.7 is out; I've wasted enough time on this already.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
|  |