Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net 1/2] net: phy: DP83822: Fix WoL in config init to be
 disabled
To:     Heiner Kallweit <hkallweit1@gmail.com>, <andrew@lunn.ch>,
        <f.fainelli@gmail.com>
CC:     <linux@armlinux.org.uk>, <davem@davemloft.net>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <afd@ti.com>
References: <20200423163947.18313-1-dmurphy@ti.com>
 <20200423163947.18313-2-dmurphy@ti.com>
 <487889d0-d2e4-ddd4-b199-c621b2826601@gmail.com>
From:   Dan Murphy <dmurphy@ti.com>
Message-ID: <54778ab0-faf0-9432-c5b8-755f1401d193@ti.com>
Date:   Thu, 23 Apr 2020 12:49:11 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <487889d0-d2e4-ddd4-b199-c621b2826601@gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Heiner

On 4/23/20 12:02 PM, Heiner Kallweit wrote:
> On 23.04.2020 18:39, Dan Murphy wrote:
>> The WoL feature should be disabled when config_init is called and the
>> feature should turned on or off  when set_wol is called.
>>
>> In addition updated the calls to modify the registers to use the set_bit
>> and clear_bit function calls.
>>
>> Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission")
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/dp83822.c | 30 ++++++++++++++++--------------
>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
>> index fe9aa3ad52a7..40fdfd043947 100644
>> --- a/drivers/net/phy/dp83822.c
>> +++ b/drivers/net/phy/dp83822.c
>> @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev,
>>   			value &= ~DP83822_WOL_SECURE_ON;
>>   		}
>>   
>> -		value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
>> -			  DP83822_WOL_CLR_INDICATION);
>> -		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>> -			      value);
>> +		/* Clear any pending WoL interrupt */
>> +		phy_read(phydev, MII_DP83822_MISR2);
>> +
>> +		value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL |
>> +			DP83822_WOL_CLR_INDICATION;
>> +
>> +		return phy_set_bits_mmd(phydev, DP83822_DEVADDR,
>> +					MII_DP83822_WOL_CFG, value);
> The switch to phy_set_bits_mmd() doesn't seem to be correct here.
> So far bit DP83822_WOL_MAGIC_EN is cleared if WAKE_MAGIC isn't set.
> Similar for bit DP83822_WOL_SECURE_ON. With your change they don't
> get cleared any longer.


Thanks for the review.  I was not watching those bits during my testing 
I will revert back to the original code.

>>   	} else {
>> -		value = phy_read_mmd(phydev, DP83822_DEVADDR,
>> -				     MII_DP83822_WOL_CFG);
>> -		value &= ~DP83822_WOL_EN;
>> -		phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG,
>> -			      value);
>> +		value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION;
>> +
> You clear one bit more than before. The reason for this may be worth a note
> in the commit message.

Thanks this was an artifact from debugging.  I can remove the CLR as it 
was cleared in set_wol enablement.

Dan