Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] net: phy: marvell: clear wol event before setting it
To:     Andrew Lunn <andrew@lunn.ch>,
        Jisheng Zhang <Jisheng.Zhang@synaptics.com>
CC:     Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Jingju Hou <Jingju.Hou@synaptics.com>
References: <20180419160232.519d15be@xhacker.debian>
 <20180419121801.GC17888@lunn.ch>
From:   Bhadram Varka <vbhadram@nvidia.com>
Message-ID: <4273f766-a017-b336-7d14-a28901d274b9@nvidia.com>
Date:   Thu, 26 Apr 2018 11:10:21 +0530
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20180419121801.GC17888@lunn.ch>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 4/19/2018 5:48 PM, Andrew Lunn wrote:
> On Thu, Apr 19, 2018 at 04:02:32PM +0800, Jisheng Zhang wrote:
>> From: Jingju Hou <Jingju.Hou@synaptics.com>
>>
>> If WOL event happened once, the LED[2] interrupt pin will not be
>> cleared unless reading the CSISR register. So clear the WOL event
>> before enabling it.
>>
>> Signed-off-by: Jingju Hou <Jingju.Hou@synaptics.com>
>> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
>> ---
>>   drivers/net/phy/marvell.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index c22e8e383247..b6abe1cbc84b 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -115,6 +115,9 @@
>>   /* WOL Event Interrupt Enable */
>>   #define MII_88E1318S_PHY_CSIER_WOL_EIE			BIT(7)
>>   
>> +/* Copper Specific Interrupt Status Register */
>> +#define MII_88E1318S_PHY_CSISR				0x13
>> +
>>   /* LED Timer Control Register */
>>   #define MII_88E1318S_PHY_LED_TCR			0x12
>>   #define MII_88E1318S_PHY_LED_TCR_FORCE_INT		BIT(15)
>> @@ -1393,6 +1396,12 @@ static int m88e1318_set_wol(struct phy_device *phydev,
>>   		if (err < 0)
>>   			goto error;
>>   
>> +		/* If WOL event happened once, the LED[2] interrupt pin
>> +		 * will not be cleared unless reading the CSISR register.
>> +		 * So clear the WOL event first before enabling it.
>> +		 */
>> +		phy_read(phydev, MII_88E1318S_PHY_CSISR);
>> +
> Hi Jisheng
>
> The problem with this is, you could be clearing a real interrupt, link
> down/up etc. If interrupts are in use, i think the normal interrupt
> handling will clear the WOL interrupt? So can you make this read
> conditional on !phy_interrupt_is_valid()?
So this will clear WoL interrupt bit from Copper Interrupt status register.

How about clearing WoL status (Page 17, register 17) for every WOL event ?

Observed that once WoL event occurred for magic packet then for next 
magic packet WoL event is not asserted.
Need to explicitly clear WOL status so that WOL interrupt will be 
generated by the HW.

Thanks,
Bhadram.


Thanks,
Bhadram