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

Mark Rutland
2015-02-17T14:51:24Z

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.