Messages in this thread |  | | Date | Tue, 17 Feb 2015 16:22:04 +0000 | From | Stathis Voukelatos <> | Subject | 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:
>> +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.
>> + >> +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?
> >> +- 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?
> >> +- 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
> >> +- 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.
>> +- 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
> >> +- 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 = <ð_controller_0>; The driver could check that it exists and is valid but does not need to make use of it.
> >> +}; >> 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
|  |