Re: [PATCH v2 1/3] Ethernet packet sniffer: Device tree binding and vendor prefix

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Feb 17, 2015 at 02:03:31PM +0000, Stathis Voukelatos wrote:
> Signed-off-by: Stathis Voukelatos <stathis.voukelatos@linn.co.uk>
> ---
>  .../bindings/net/linn-ether-packet-sniffer.txt     | 42 ++++++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>  2 files changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt b/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt
> new file mode 100644
> index 0000000..74bac5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/linn-ether-packet-sniffer.txt
> @@ -0,0 +1,42 @@
> +* Linn Products Ethernet Packet Sniffer
> +The module allows Ethernet packets to be parsed, matched against
> +a user-defined pattern and timestamped. It sits between a 100M
> +Ethernet MAC and PHY and is completely passive with respect to
> +Ethernet frames.
> +Matched packet bytes and timestamp values are returned through a
> +FIFO. Timestamps are provided to the module through an externally
> +generated Gray-encoded counter.

Does this counter unit need to be enabled (or have any input clocks
enabled)?

> +
> +Required properties:
> +- compatible : must be "linn,eth-sniffer"

Is there not a more precise name for this IP block?

> +- reg : physical addresses and sizes of registers. Must contain 3 entries:
> +          - registers memory space
> +          - TX command string memory
> +          - RX command string memory
> +- reg-names : must contain the following 3 entries:
> +                  "regs", "tx-ram", "rx-ram"

It would be nicer to format this as:

- reg: A list of physical address and size pairs corresponding to each
       entry in reg-names

- reg-names: must contain:
  * "regs" for the control registers
  * "tx-ram" for the TX command string memory
  * "rx-ram" for the RX command string memory

Which avoids redundancy.

The phrase "command string" sounds a bit odd. What are these used for
exactly?

> +- interrupts : sniffer interrupt specifier
> +- clocks : specify the system clock for the peripheral and
> +	   the enable clock for the timestamp counter
> +- clock-names : must contain the "sys" and "tstamp" entries

Similarly here clocks should just be defined in terms of clock-names.

> +- fifo-block-words : number of words in one data FIFO entry

I didn't see a data FIFO described. Is that dynamically allocated and
handed to the sniffer, or does that correspond to one of the memory
regions above?

> +- tstamp-hz : frequency of the timestamp counter
> +- tstamp-shift : shift value for the timestamp cyclecounter struct

What exactly is this used for?

Are there any docs?

> +- tstamp-bits : width in bits of the timestamp counter
> +
> +Example:
> +
> +sniffer@1814a000 {
> +	compatible = "linn,eth-sniffer";
> +	reg = <0x1814a000 0x100>, <0x1814a400 0x400>,
> +	      <0x1814a800 0x400>;
> +	reg-names = "regs", "tx-ram", "rx-ram";
> +	interrupts = <GIC_SHARED 58 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&clk_core CLK_AUDIO>,
> +		 <&cr_periph SYS_CLK_EVENT_TIMER>;
> +	clock-names = "sys", "tstamp";
> +	fifo-block-words = <4>;
> +	tstamp-hz = <52000000>;
> +	tstamp-shift = <27>;
> +	tstamp-bits = <30>;

This property wasn't documented.

As mentioned previously, I think the relation between this unit and the
MAC and/or PHY needs to be explicitly described in the DT.

> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index d443279..891c224 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -90,6 +90,7 @@ lacie	LaCie
>  lantiq	Lantiq Semiconductor
>  lenovo	Lenovo Group Ltd.
>  lg	LG Corporation
> +linn    Linn Products Ltd.

This addition looks fine to me. For some reason it seems to be padded
with spaces instead of tabs though; is my mail server corrupting things
or is that the case in the original patch?

Thanks,
Mark.