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

Hi Mark,

On 17/02/15 14:51, Mark Rutland wrote:

quoted

+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)?

Yes it does, that is the purpose of the 'tstamp' entry in the
'clock-names' property below.

quoted

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

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

It is generally called 'ethernet packet sniffer', maybe
linn,eth-packet-sniffer would be a more descriptive name?

quoted

+- 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.

Will change formatting as suggested

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

In these two memory areas we program a sequence of bytes in the
format: [cmd][value][cmd][value] to configure the data patterns that
the sniffer should match for Ethernet TX and RX packets respectively.
Maybe 'command memory' would be clearer?

quoted

+- 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.

Will reformat similar to the 'regs' field

quoted

+- 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?

It is a H/W FIFO internal to the module and accessed through
a register. It is divided in blocks and 'fifo-block-words'
specify the number of words in each block. It is needed by
the driver to make sure it reads an entire block, in order
to clear the 'data available' interrupt.

quoted

+- 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?

See: include/linux/clocksource.h
The driver uses a cyclecounter and timecounter to convert raw timestamps
to nanoseconds. 'tstamp-shift' refers to the 'shift' field of the
cyclecounter structure, that can be used to improve the precision of
the conversion

quoted

+- 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.

Do you suggest a field along the lines of:
mac = <&eth_controller_0>;
The driver could check that it exists and is valid but does
not need to make use of it.

quoted

+};

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?

Sorry, it was spaces. Will be fixed

Thanks,
Mark.

Thank you,
Stathis