netdev - Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

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:   Thu, 13 Dec 2018 17:43:19 +0100
From:   Marek Vasut <marex@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, f.fainelli@...il.com
Subject: Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

On 12/13/2018 03:33 PM, Andrew Lunn wrote:
> On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
>> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
>>>> +static int tja11xx_config_aneg(struct phy_device *phydev)
>>>> +{
>>>> +	phydev->autoneg = 0;
>>>> +	phydev->speed = SPEED_100;
>>>> +	phydev->duplex = DUPLEX_FULL;
>>>> +	phydev->pause = 0;
>>>> +	phydev->asym_pause = 0;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Hi Marek
>>
>> Hi,
>>
>>> That is, err, interesting. Are you saying the PHY cannot do auto-neg?
>>
>> There isn't much to auto-negotiate and some of the bits in the standard
>> registers behave weirdly.
> 
> Hi Marek

Hi,

> Hopefully the config_aneg callback is not called if you don't list
> autoneg to the .features. The microchip_t1 driver just uses
> genphy_config_aneg, but if a NULL works, i would prefer that.

Without the custom config_aneg which sets speed and duplex, I get a
report claiming the link is at 10/Half , while the link is at 100/Full.
If I force this in the custom config_aneg, the communication works fine.
Do you have a hint for me ?

>>> Does it happen to be a 100T1 device for automotive?
>>
>> Yes, that's what the commit message says, why ?
> 
> Sorry, i missed it in the commit message.  We now have enough bits in
> the link_mode that we should explicitly say this is 100BaseT2_Full,
> not 100BaseT_Full. So please add a new member to
> ethtool_link_mode_bit_indices ...

Oh, nice. Shouldn't this be basic_t1 , which is already supported ?

>>>> +
>>>> +static int tja11xx_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
> 
> And make it part of the .features. Well, you need to add a new
> PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
> phy_driver structure. There should not be any need to modify it in the
> config_init call.

Right

> We also need to think about what we do with the PHY_BASIC_T1_FEATURES
> macro. Ideally we want to swap that to also make use of a new
> ethtool_link_mode_bit_indices, but i've no idea at the moment if that
> will break something.

Do you mind if I skip this part for now , until I get the driver into
better shape ?

-- 
Best regards,
Marek Vasut