mvneta: SGMII fixed-link not so fixed

[stsp@list.ru (Stas Sergeev)]

18.09.2015 18:43, Russell King - ARM Linux ?????:
> On Fri, Sep 18, 2015 at 05:45:27PM +0300, Stas Sergeev wrote:
>> 18.09.2015 16:57, Russell King - ARM Linux ?????:
>>> On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
>>>> 18.09.2015 16:12, Russell King - ARM Linux ?????:
>>>>> On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
>>>>>> 18.09.2015 15:13, Russell King - ARM Linux ?????:
>>>>>>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
>>>>>>>> 18.09.2015 02:14, Russell King - ARM Linux ?????:
>>>>>>>>>  _But_ using the in-band status
>>>>>>>>>    property fundamentally requires this for mvneta to behave correctly:
>>>>>>>>>
>>>>>>>>> 		phy-mode = "sgmii";
>>>>>>>>> 		managed = "in-band-status";
>>>>>>>>> 		fixed-link {
>>>>>>>>> 		};
>>>>>>>>>
>>>>>>>>>    with _no_ phy node.
>>>>>>>> I don't understand this one.
>>>>>>>> At least for me it works without empty fixed-link.
>>>>>>>> There may be some bug.
>>>>>>>
>>>>>>>         if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
>>>>>>>                 u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
>>>>>>>
>>>>>>>                 mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
>>>>>>>                 if (pp->use_inband_status && (cause_misc &
>>>>>>>                                 (MVNETA_CAUSE_PHY_STATUS_CHANGE |
>>>>>>>                                  MVNETA_CAUSE_LINK_CHANGE |
>>>>>>>                                  MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
>>>>>>>                         mvneta_fixed_link_update(pp, pp->phy_dev);
>>>>>>>                 }
>>>>>>>
>>>>>>> pp->use_inband_status is set when managed = "in-band-status" is set.
>>>>>>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
>>>>>>>
>>>>>>> mvneta_fixed_link_update() reads the status, and communicates that into
>>>>>>> the fixed-link phy:
>>>>>>>
>>>>>>>         u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
>>>>>>>
>>>>>>> 	... code setting status.* values from gmac_stat ...
>>>>>>>         changed.link = 1;
>>>>>>>         changed.speed = 1;
>>>>>>>         changed.duplex = 1;
>>>>>>> 	fixed_phy_update_state(phy, &status, &changed);
>>>>>>>
>>>>>>> fixed_phy_update_state() then looks up the phy in its list, comparing only
>>>>>>> the address:
>>>>>>>
>>>>>>>         if (!phydev || !phydev->bus)
>>>>>>>                 return -EINVAL;
>>>>>>>
>>>>>>>         list_for_each_entry(fp, &fmb->phys, node) {
>>>>>>>                 if (fp->addr == phydev->addr) {
>>>>>>>
>>>>>>> updating fp->* with the new state, calling fixed_phy_update_regs().  This
>>>>>>> updates the fixed-link phy emulated registers, and phylib then notices
>>>>>>> the change in link status, and notifies the netdevice attached to the
>>>>>>> PHY it found of the change.
>>>>>>>
>>>>>>> Now, one of two things happens as a result of this:
>>>>>>>
>>>>>>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
>>>>>>>    phy to update its "fixed-link" properties, and the "not so fixed" phy
>>>>>>>    changes its parameters according to the new status.
>>>>>>>
>>>>>>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
>>>>>>>    phy,
>>>>>> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
>>>>>> I don't think MDIO PHYs can appear there. If they can - the bug is
>>>>>> very nasty. Have you seen exactly when/why that happens?
>>>>>
>>>>> I think I explained it fully - please follow the code paths I've detailed
>>>>> above.
>>>> I try. What I don't understand is why both PHYs get the
>>>> same address on the "Fixed MDIO bus".
>>>
>>> They aren't both on the fixed MDIO bus - that's the whole point I'm
>>> making.  They get the same phydev->addr but on _different_ buses.
>>
>> So you have an MDIO phy for which mvneta seems to have
>> use_inband_status==true, correct?
> 
> That is one very real possibililty.  Cisco SGMII in-band status is for a
> SGMII PHY to inform the MAC about the properties of the link to which the
> PHY is attached.
> 
> So, specifying "managed = in-band-status" along with a real PHY is very
> much a _real_ possibility, as I've previously explained.
> 
>> AFAICS if it has use_inband_status==true,
>> then it went through of_phy_register_fixed_link(dn),
> 
> That's totally incorrect.  The test for setting use_inband_status in
> mvneta is:
> 
>         err = of_property_read_string(dn, "managed", &managed);
>         pp->use_inband_status = (err == 0 &&
>                                  strcmp(managed, "in-band-status") == 0);

Arrrr! I was looking at the branch without the last
patch applied, so it occurred to me as

	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
		fixed_phy;

Sorry for that.
So we seem to indeed have a nasty regression with the patch
that just went to stable. :( Great news.
Thanks for you time.

I still have problems with this part though:
> If there's neither a MDIO PHY nor a fixed-link, then the network driver
> fails to initialise the device.

I think I am looking into the right source this time, seems like
if we don't have both but still have managed="in-band-status", that
should go the fixed-link path and still work... no?