Hi Harini,
On 10.05.2018 13:37, Harini Katakam wrote:
> Hi Claudiu,
>
> On Fri, May 4, 2018 at 5:47 PM, Claudiu Beznea
> <claudiu.bez...@microchip.com> wrote:
>>
>>
>> On 22.03.2018 15:51, harinikatakamli...@gmail.com wrote:
>>> From: Harini Katakam <hari...@xilinx.com>
>>>
>>> This patch enables ARP wake event support in GEM through the following:
>>>
>>> -> WOL capability can be selected based on the SoC/GEM IP version rather
>>> than a devictree property alone. Hence add a new capability property and
>>> set device as "wakeup capable" in probe in this case.
>>> -> Wake source selection can be done via ethtool or by enabling wakeup
>>> in /sys/devices/platform/..../ethx/power/
>>> This patch adds default wake source as ARP and the existing selection of
>>> WOL using magic packet remains unchanged.
>>> -> When GEM is the wake device with ARP as the wake event, the current
>>> IP address to match is written to WOL register along with other
>>> necessary confuguration required for MAC to recognize an ARP event.
>>> -> While RX needs to remain enabled, there is no need to process the
>>> actual wake packet - hence tie off all RX queues to avoid unnecessary
>>> processing by DMA in the background.
>>
>> Why is this different for magic packet vs ARP packet?
>
> This should ideally be the same whether it is a magic packet or ARP on
> the version of the IP we use (more details in my comment below).
> I simply did not alter the magic packet code for now to avoid breaking
> others' flow.
I see your point. But in the end the code should be nice and clean.
>
> <snip>
>>> +#define MACB_CAPS_WOL 0x00000080
>>
>> I think would be better to have this as part of bp->wol and use it properly
>> in suspend/resume hooks.
>
> I think a capability flag as part of config structure is better
> because this is clearly an SoC related feature and there is no need
> to have a devicetree property.
Since there is no difference b/w ARP and magic packet in term of SoC,
I mean, there is no special treatment b/w them, I think we should treat
them both in the same way. This was my point.
>
> <snip>
>> Wouldn't it work if you will change it in something like this:
>>
>> u32 wolmask, arpipmask = 0;
>>
>> if (bp->wol & MACB_WOL_ENABLED) {
>> macb_writel(bp, IER, MACB_BIT(WOL));
>>
>> if (bp->wol & MACB_WOL_HAS_ARP_PACKET) {
>> /* Enable broadcast. */
>> gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) &
>> ~MACB_BIT(NBC));
>> arpipmask =
>> cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local) & 0xFFFF;
>> wolmask = arpipmask | MACB_BIT(ARP);
>> } else {
>> wolmask = MACB_BIT(MAG);
>> }
>>
>> macb_writel(bp, WOL, wolmask);
>> enable_irq_wake(bp->queues[0].irq);
>
> The above would work. But I'd still have to add the RX BD changes
> and then stop the phy, disable interrupt etc., for most optimal power
> down state - the idea is to keep only is essential to detect a wake event.
>
Also, with your approach the suspend/resume time will be longer.
>> netif_device_detach(netdev);
>> }
>>
>> I cannot find anything particular for ARP WOL events in datasheet. Also,
>> I cannot find something related to DMA activity while WOL is active
>
> Can you please let me know which version you are referring to?
> ZynqMP uses the IP version r1p06 or 07.
I have user guide revision 15
.
>
> There is a clear set of rules for ARP wake event to be recognized.
>
> About the DMA activity, it is not explicitly mentioned but we have
> observed that the DMA will continue to process incoming packets
> and try to write them to the memory and Cadence has confirmed
> the same. Later versions of the IP may have some provision to
> stop DMA activity on RX channel but unfortunately in this version,
> using a dummy RX buffer descriptor is the only way.>
> I'm looking into your other suggestions.
> Thanks, will get back to you.
>
> Regards,
> Harini
>
