[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150217145124.GM8994@leverpostej>
Date: Tue, 17 Feb 2015 14:51:24 +0000
From: Mark Rutland <mark.rutland@....com>
To: Stathis Voukelatos <stathis.voukelatos@...n.co.uk>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"abrestic@...omium.org" <abrestic@...omium.org>
Subject: Re: [PATCH v2 1/3] Ethernet packet sniffer: Device tree binding and
vendor prefix
On Tue, Feb 17, 2015 at 02:03:31PM +0000, Stathis Voukelatos wrote:
> Signed-off-by: Stathis Voukelatos <stathis.voukelatos@...n.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@...4a000 {
> + 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists