Re: [PATCH net-next 2/2] net: phy: Add PHY Auto/Mdi/Mdix set driver for Microsemi PHYs.

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/05/2016 12:18 AM, Andrew Lunn wrote:
>>>> +     phydev->mdix = ETH_TP_MDI_AUTO;
>>>
>>> Humm, interesting. The only other driver supporting mdix is the
>>> Marvell one. It does not do this, it leaves it to its default value of
>>> ETH_TP_MDI_INVALID. It does however interpret ETH_TP_MDI_INVALID as
>>> meaning as ETH_TP_MDI_AUTO.
>>>
>>> There needs to be consistency here. You either need to do the same as
>>> the Marvell driver, or you need to modify the Marvell driver to also
>>> set phydev->mdix to ETH_TP_MDI_AUTO.
>>>
>> In Ethtool two variable i.e. eth_tp_mdix_ctrl, eth_tp_mdix use to update
>> the status. But, driver header is having one variable i.e. mdix.
>> Driver header should also have another variabl like mdix_ctrl.
>> Then, Ethtool can get/set the Auto MDIX/MDI.
>> In case, mdix is not configure with ETH_TP_MDI_AUTO, Ethtool shows error as
>> "setting MDI not supported"

Agreed, we currently report eth_tp_mdi and eth_tp_mdi_ctrl using
phydev->mdix, but this is too limiting.

>>
>> Please suggest me if you have any better method to fix this issue.
> 
> Maybe we should add a new flag for the .flags member of the
> phy_driver. If PHY_HAS_MDIX is set, the phy core will set phydev->mdix
> to ETH_TP_MDI_AUTO?

I agree with Raju here, like most other Ethernet drivers, we should
allow PHY drivers to have an eth_tp_mdix_ctrl to indicate what is the
configured MDI setting, and read eth_tp_mdi to indicate what is the
current status, then ethtool can properly differentiate what is going on.

Raju, Andrew, does that work for you?
-- 
Florian