Hi Gerlando,
Le lundi 10 février 2014, 17:14:59 Gerlando Falauto a écrit :
> Hi,
>
> I'm currently trying to fix an issue for which this patch provides a
> partial solution, so apologies in advance for jumping into the
> discussion for my own purposes...
>
> On 02/04/2014 09:39 PM, Florian Fainelli wrote:> 2014-01-17 Matthew
>
> Garrett <matthew.garrett@xxxxxxxxxx>:
> >> Some hardware may be broken in interesting and board-specific ways, such
> >> that various bits of functionality don't work. This patch provides a
> >> mechanism for overriding mii registers during init based on the
>
> contents of
>
> >> the device tree data, allowing board-specific fixups without having to
> >> pollute generic code.
> >
> > It would be good to explain exactly how your hardware is broken
> > exactly. I really do not think that such a fine-grained setting where
> > you could disable, e.g: 100BaseT_Full, but allow 100BaseT_Half to
> > remain usable makes that much sense. In general, Gigabit might be
> > badly broken, but 100 and 10Mbits/sec should work fine. How about the
> > MASTER-SLAVE bit, is overriding it really required?
> >
> > Is not a PHY fixup registered for a specific OUI the solution you are
> > looking for? I am also concerned that this creates PHY troubleshooting
> > issues much harder to debug than before as we may have no idea about
> > how much information has been put in Device Tree to override that.
> >
> > Finally, how about making this more general just like the BCM87xx PHY
> > driver, which is supplied value/reg pairs directly? There are 16
> > common MII registers, and 16 others for vendor specific registers,
> > this is just covering for about 2% of the possible changes.
>
> Good point. That would easily help me with my current issue, which
> requires autoneg to be disabled to begin with (by clearing BMCR_ANENABLE
> from register 0).
Is there a point in time (e.g: after some specific initial configuration has
been made) where BMCR_ANENABLE can be used?
> This would not however fix it entirely (I tried a quick hardwired
> implementation), as the whole PHY machinery would not take that into
> account and would re-enable autoneg anyway.
> I also tried changing the patch so that phydev->support gets updated
There are multiple things that you could try doing here:
- override the PHY state machine in your read_status callback to make sure
that you always set phydev->autoneg set to AUTONEG_ENABLE
- clear the SUPPORTED_Autoneg bits from phydev->supported right after PHY
registration and before the call to phy_start()
- set the PHY_HAS_MAGICANEG bit in your PHY driver flag
>
> (instead of phydev->advertising):
> >> + if (!of_property_read_u32(np, override->prop, &tmp)) {
> >> + if (tmp) {
> >> + *val |= override->value;
> >> + phydev->advertising |=
>
> override->supported;
>
> >> + } else {
> >> + phydev->advertising &=
>
> ~(override->supported);
>
> >> + }
> >> +
> >> + *mask |= override->value;
>
> What I find weird is that the only way phydev->autoneg could ever be set
> to disabled is from here (phy.c):
>
> static void phy_sanitize_settings(struct phy_device *phydev)
> {
> u32 features = phydev->supported;
> int idx;
>
> /* Sanitize settings based on PHY capabilities */
> if ((features & SUPPORTED_Autoneg) == 0)
> phydev->autoneg = AUTONEG_DISABLE;
>
> which is in turn only called when phydev->autoneg is set to
> AUTONEG_DISABLE to begin with:
>
> int phy_start_aneg(struct phy_device *phydev)
> {
> int err;
>
> mutex_lock(&phydev->lock);
>
> if (AUTONEG_DISABLE == phydev->autoneg)
> phy_sanitize_settings(phydev);
>
> So could someone please help me figure out what I'm missing here?
At first glance it looks like the PHY driver should be reading the phydev-
>autoneg value when the PHY driver config_aneg() callback is called to be
allowed to set the forced speed and settings.
The way phy_sanitize_settings() is coded does not make it return a mask of
features, but only the forced supported speed and duplex. Then when the link
is forced but we are having some issues getting a link status, libphy tries
lower speeds and this function is used again to provide the next speed/duplex
pair to try.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
