lkml.org 
[lkml]   [2015]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 = <&eth_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



    \
     
     \ /
      Last update: 2015-02-17 17:41    [W:3.966 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site