[lkml]   [2018]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC PATCH 5/5] net: macb: Add WOL support with ARP
Hi Harini,

On 10.05.2018 13:37, Harini Katakam wrote:
> Hi Claudiu,
> On Fri, May 4, 2018 at 5:47 PM, Claudiu Beznea
> <> wrote:
>> On 22.03.2018 15:51, wrote:
>>> From: Harini Katakam <>
>>> 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

 \ /
  Last update: 2018-05-15 10:40    [W:0.099 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site