netdev - Re: [PATCH net-next] net: phy: add basic driver for NXP CBTX PHY

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] [day] [month] [year] [list]

Message-ID: <a1903ab4-1ef9-5bf5-55a1-6e039ca7712f@gmail.com>
Date:   Thu, 13 Apr 2023 21:46:29 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next] net: phy: add basic driver for NXP CBTX PHY

On 13.04.2023 15:51, Vladimir Oltean wrote:
> Hi Heiner,
> 
> On Tue, Apr 11, 2023 at 06:49:42PM +0200, Heiner Kallweit wrote:
>>> +	return phy_set_bits(phydev, CBTX_PDOWN_CTRL,
>>> +			    CBTX_PDOWN_CTL_TRUE_PDOWN);
>>
>> A comment may be helpful explaining how true_pdown mode
>> is different from power-down mode set by C22 standard
>> bit in BMCR as part of genphy_suspend().
> 
> The NXP documentation for True Power Down vs General Power Down did not
> convince me, and I don't want to speculate and give an answer that might
> be incorrect.
> 
> There are not many people who can help me give an answer right now
> during the holidays. The high level idea is that the PHY may enter a
> mode of lower power consumption.
> 
> If it's acceptable to you, I can implement suspend and resume as direct
> calls to genphy_suspend() and genphy_resume(), and make the change later,
> if needed.
> 
No, it's no blocker for me. It may just be useful to know which additional
blocks are powered down and whether True Power Down comes with certain
constraints like e.g. parts of the register set not being accessible or
not reacting.

>>> +static int cbtx_config_intr(struct phy_device *phydev)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
>>> +		ret = cbtx_ack_interrupts(phydev);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		ret = phy_write(phydev, CBTX_IRQ_ENABLE,
>>> +				CBTX_IRQ_AN_COMPLETE | CBTX_IRQ_LINK_DOWN);
>>
>> I think you need also CBTX_IRQ_ENERGYON. Otherwise you won't get a
>> link-up interrupt in forced mode.
> 
> I've tested just now with "ethtool -s p1 autoneg off speed 100 duplex full",
> and you are exactly correct. I will add the media side energy detection logic
> to the enabled IRQ sources in v2. Thanks.