Re: [PATCH net-next 2/3] dt-bindings: net: cdns,macb: Add wol-arp-packet property

[Vineeth Karumanchi]

Hi Conor,

On 30/01/24 11:00 pm, Conor Dooley wrote:
> On Tue, Jan 30, 2024 at 04:18:44PM +0530, Vineeth Karumanchi wrote:
> > "wol-arp-packet" property enables WOL with ARP packet.
> > It is an extension to "magic-packet for WOL.
>
> If it is an extension to "magic-packet" why does it not depend on
> "magic-packet"? Are there systems that would only support the magic arp
> packet but a regular magic packet?

The IP version on ZU+ and Versal supports the below combinations for WOL 
event:

1. Magic packet (Wake-on magic packet only)
2. ARP (Wake-on ARP packet only)
3. Magic packet or ARP (Wake-on magic or ARP packets)

The existing DT binding already has one entry for
wol via magic packet. We are adding ARP packet support to the existing 
implementation.

I will change the commit message in v2.

> > Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@xxxxxxx>
> > ---
> > 7c4a1d0cfdc1 net: macb: make magic-packet property generic
> > which added magic-property support and wol-arp-packet addition
> > is similar extension.
> > ---
> >   Documentation/devicetree/bindings/net/cdns,macb.yaml | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> > index bf8894a0257e..4bea177e85bc 100644
> > --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> > +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> > @@ -144,6 +144,11 @@ patternProperties:
> >           description:
> >             Indicates that the hardware supports waking up via magic packet.
> >   
> > +      wol-arp-packet:
>
> Bikeshedding perhaps, but why not call it "magic-arp-packet" if it has
> the same function as the other property here?

Magic packet and ARP packets are two different wol events.
IP supports configuring in the above-mentioned ways.
Hence, I think it would be good to not mix with magic packet.

Please let me know your suggestions/comments.

Thanks,
Vineeth 🙏

> Thanks,
> Conor.
>
> > +        type: boolean
> > +        description:
> > +          Indicates that the hardware supports waking up via ARP packet.
> > +
> >       unevaluatedProperties: false
> >   
> >   required:
> > --
> > 2.34.1