diff mbox

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

Message ID 0a86907642a97e5bd880f69299664232fcffaf9d.1424181053.git.stathis.voukelatos@linn.co.uk
State Needs Review / ACK, archived
Headers show

Checks

Context Check Description
robh/checkpatch warning total: 1 errors, 0 warnings, 0 lines checked
robh/patch-applied success

Commit Message

Stathis Voukelatos Feb. 17, 2015, 2:03 p.m. UTC
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

Comments

Sergei Shtylyov Feb. 17, 2015, 2:16 p.m. UTC | #1
Hello.

On 02/17/2015 05:03 PM, Stathis Voukelatos wrote:

> Signed-off-by: Stathis Voukelatos <stathis.voukelatos@linn.co.uk>

[...]

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

    Please use tab for indenting the company name.

>   linux	Linux-specific binding
>   lsi	LSI Corp. (LSI Logic)
>   lltc	Linear Technology Corporation

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Feb. 17, 2015, 2:51 p.m. UTC | #2
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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stathis Voukelatos Feb. 17, 2015, 4:22 p.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Feb. 17, 2015, 4:35 p.m. UTC | #4
On Tue, Feb 17, 2015 at 04:22:04PM +0000, Stathis Voukelatos wrote:
> 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.

Ah, I see.

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

Either way is fine. I was expecting something more like a product code.

[...]

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

I see. I assumed that the FIFO was memory mapped rather than exposed
through a register.

Given the above this sounds fine.

> >> +- tstamp-hz : frequency of the timestamp counter

Is this the frequency the clock is running at, or a frequency that it
should be programmed to in order to be used?

The former can be queried from the common clock framework, and if you
intended the latter the wording shuold be a little more explicit about
that being the case.

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

Sure, but the very concept of a cyclecounter is a Linux implementation
detail. If we have the frequency of the timer we should be able to
dynamically generate this, so there's no need for this to be in the DT.

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

I would expect some level of the software stack to make use of it, or
you have no idea which ethernet interface is related to this monitoring
interface. Perhaps current systems only have one interface, but that
shouldn't be relied upon.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stathis Voukelatos Feb. 17, 2015, 5:13 p.m. UTC | #5
Hi Mark,

On 17/02/15 16:35, Mark Rutland wrote:
>
>>>> +- tstamp-hz : frequency of the timestamp counter
>
> Is this the frequency the clock is running at, or a frequency that it
> should be programmed to in order to be used?
>
> The former can be queried from the common clock framework, and if you
> intended the latter the wording shuold be a little more explicit about
> that being the case.
>

It is the frequency of the timestamp values supplied to the sniffer
module. It is used by the driver to convert to nanoseconds.
I was trying to be somewhat generic here and not assume that it
is necessarily the same as the 'tstamp' clock below, in which case we
could indeed obtain it using the common clock framework.

>> 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
>
> Sure, but the very concept of a cyclecounter is a Linux implementation
> detail. If we have the frequency of the timer we should be able to
> dynamically generate this, so there's no need for this to be in the DT.
>

Most networking driver use hard-coded values for that, but in my case
I did not want to assume a certain fixed clock frequency. I will remove
it from the DT and generate it dynamically. There is a kernel function
clocks_calc_mult_shift() to do it but unfortunately it is not exported,
so I guess I will need to replicate the code.

>>> 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.
>
> I would expect some level of the software stack to make use of it, or
> you have no idea which ethernet interface is related to this monitoring
> interface. Perhaps current systems only have one interface, but that
> shouldn't be relied upon.

Yes, but the sniffer module is hard-wired to a certain Ethernet Mii
interface. We can add an entry to tie it to an Ethernet controller, but
apart of a sanity check I am not sure what else the S/W can do.

>
> Thanks,
> Mark.
>

Thank you,
Stathis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Feb. 17, 2015, 5:30 p.m. UTC | #6
On Tue, Feb 17, 2015 at 05:13:19PM +0000, Stathis Voukelatos wrote:
> Hi Mark,
> 
> On 17/02/15 16:35, Mark Rutland wrote:
> >
> >>>> +- tstamp-hz : frequency of the timestamp counter
> >
> > Is this the frequency the clock is running at, or a frequency that it
> > should be programmed to in order to be used?
> >
> > The former can be queried from the common clock framework, and if you
> > intended the latter the wording shuold be a little more explicit about
> > that being the case.
> >
> 
> It is the frequency of the timestamp values supplied to the sniffer
> module. It is used by the driver to convert to nanoseconds.
> I was trying to be somewhat generic here and not assume that it
> is necessarily the same as the 'tstamp' clock below, in which case we
> could indeed obtain it using the common clock framework.

In what cases would it _not_ be the same? From your description this is
that clock, no?

> >> 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
> >
> > Sure, but the very concept of a cyclecounter is a Linux implementation
> > detail. If we have the frequency of the timer we should be able to
> > dynamically generate this, so there's no need for this to be in the DT.
> >
> 
> Most networking driver use hard-coded values for that, but in my case
> I did not want to assume a certain fixed clock frequency. I will remove
> it from the DT and generate it dynamically. There is a kernel function
> clocks_calc_mult_shift() to do it but unfortunately it is not exported,
> so I guess I will need to replicate the code.

Or submit a patch exporting it, along with the rationale for doing so?

> >>> 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.
> >
> > I would expect some level of the software stack to make use of it, or
> > you have no idea which ethernet interface is related to this monitoring
> > interface. Perhaps current systems only have one interface, but that
> > shouldn't be relied upon.
> 
> Yes, but the sniffer module is hard-wired to a certain Ethernet Mii
> interface. We can add an entry to tie it to an Ethernet controller, but
> apart of a sanity check I am not sure what else the S/W can do.

Fundamentally, the use-case for this is monitoring an ethernet
interface. So regardless of which kernel framework this plumbs into,
there needs to be a way to go from ethN to whatever this is exposed as.

Exposing a completely separate interface makes no sense. Singleton stuff
like that inevitably gets broken as someone later builds a board with
multiple instances of some similar IP block.

So I would imagine that either the link between interface and monitoring
interface would be described somewhere in the filesystem, or there'd be
a syscall/ioctl/whatever to go from an interface to the appropriate
monitoring interface.

That all depends on exactly how this gets exposed in the end, however.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stathis Voukelatos Feb. 18, 2015, 9:40 a.m. UTC | #7
On 17/02/15 17:30, Mark Rutland wrote:

>> It is the frequency of the timestamp values supplied to the sniffer
>> module. It is used by the driver to convert to nanoseconds.
>> I was trying to be somewhat generic here and not assume that it
>> is necessarily the same as the 'tstamp' clock below, in which case we
>> could indeed obtain it using the common clock framework.
>
> In what cases would it _not_ be the same? From your description this is
> that clock, no?
>

Counters can often have a divider applied to their input clock and
therefore run at a scaled down frequency. This is not the case in the
first SoC where the sniffer is used, so for simplicity I can modify as
you suggest and remove that field from the DT.

>> Most networking driver use hard-coded values for that, but in my case
>> I did not want to assume a certain fixed clock frequency. I will remove
>> it from the DT and generate it dynamically. There is a kernel function
>> clocks_calc_mult_shift() to do it but unfortunately it is not exported,
>> so I guess I will need to replicate the code.
>
> Or submit a patch exporting it, along with the rationale for doing so?
>

Will do that.

>> Yes, but the sniffer module is hard-wired to a certain Ethernet Mii
>> interface. We can add an entry to tie it to an Ethernet controller, but
>> apart of a sanity check I am not sure what else the S/W can do.
>
> Fundamentally, the use-case for this is monitoring an ethernet
> interface. So regardless of which kernel framework this plumbs into,
> there needs to be a way to go from ethN to whatever this is exposed as.
>
> Exposing a completely separate interface makes no sense. Singleton stuff
> like that inevitably gets broken as someone later builds a board with
> multiple instances of some similar IP block.
>
> So I would imagine that either the link between interface and monitoring
> interface would be described somewhere in the filesystem, or there'd be
> a syscall/ioctl/whatever to go from an interface to the appropriate
> monitoring interface.
>
> That all depends on exactly how this gets exposed in the end, however.
>

After the first version of the patch was submitted, the feedback from 
the netdev list was to expose it as a network interface as this would
allow it to be accessed by standard user space monitoring tools.
It definitely makes sense to link it to the associated ethernet netdev,
but I am not sure if there is a framework in the kernel to do it at
the driver level?

> Thanks,
> Mark.
> --

Thank you,
Stathis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Feb. 18, 2015, 12:11 p.m. UTC | #8
On Wed, Feb 18, 2015 at 09:40:22AM +0000, Stathis Voukelatos wrote:
> 
> 
> On 17/02/15 17:30, Mark Rutland wrote:
> 
> >> It is the frequency of the timestamp values supplied to the sniffer
> >> module. It is used by the driver to convert to nanoseconds.
> >> I was trying to be somewhat generic here and not assume that it
> >> is necessarily the same as the 'tstamp' clock below, in which case we
> >> could indeed obtain it using the common clock framework.
> >
> > In what cases would it _not_ be the same? From your description this is
> > that clock, no?
> >
> 
> Counters can often have a divider applied to their input clock and
> therefore run at a scaled down frequency. This is not the case in the
> first SoC where the sniffer is used, so for simplicity I can modify as
> you suggest and remove that field from the DT.

The common clock bindings have fixed-factor-clock for handling dividers,
so I believe you should be able to use that.

You mentioned that the counter was a block external to the sniffer. Does
it have any configuration interface (e.g. to reset the counter)? We may
need to model it in the DT if so (and describe the clock as feeding into
it rather than into the sniffer).

> >> Yes, but the sniffer module is hard-wired to a certain Ethernet Mii
> >> interface. We can add an entry to tie it to an Ethernet controller, but
> >> apart of a sanity check I am not sure what else the S/W can do.
> >
> > Fundamentally, the use-case for this is monitoring an ethernet
> > interface. So regardless of which kernel framework this plumbs into,
> > there needs to be a way to go from ethN to whatever this is exposed as.
> >
> > Exposing a completely separate interface makes no sense. Singleton stuff
> > like that inevitably gets broken as someone later builds a board with
> > multiple instances of some similar IP block.
> >
> > So I would imagine that either the link between interface and monitoring
> > interface would be described somewhere in the filesystem, or there'd be
> > a syscall/ioctl/whatever to go from an interface to the appropriate
> > monitoring interface.
> >
> > That all depends on exactly how this gets exposed in the end, however.
> >
> 
> After the first version of the patch was submitted, the feedback from 
> the netdev list was to expose it as a network interface as this would
> allow it to be accessed by standard user space monitoring tools.
> It definitely makes sense to link it to the associated ethernet netdev,
> but I am not sure if there is a framework in the kernel to do it at
> the driver level?

Unfortunately I'm not familiar enough with the net code; hopefully
someone else can point us in the right direction.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stathis Voukelatos Feb. 18, 2015, 1:56 p.m. UTC | #9
Hi Mark,

On 18/02/15 12:11, Mark Rutland wrote:
>>
>> Counters can often have a divider applied to their input clock and
>> therefore run at a scaled down frequency. This is not the case in the
>> first SoC where the sniffer is used, so for simplicity I can modify as
>> you suggest and remove that field from the DT.
>
> The common clock bindings have fixed-factor-clock for handling dividers,
> so I believe you should be able to use that.
>
> You mentioned that the counter was a block external to the sniffer. Does
> it have any configuration interface (e.g. to reset the counter)? We may
> need to model it in the DT if so (and describe the clock as feeding into
> it rather than into the sniffer).
>

The sniffer module is designed so that it receives a Gray encoded
timestamp from another module of the SoC that it is integrated in.
The first integration that we have is in the IMG Pistachio SoC.
There the timestamp counter is part of the Event Timer module
and its source is one of the system clocks of the chip (configurable)
That module will eventually have it's own device-tree node when a
driver becomes available for it.

The 'tstamp' clock given in the sniffer DT node should be such
that when it is enabled it will start the counter and its parent clock,
so that it can start counting.

> Thanks,
> Mark.
>

Stathis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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.
+
+Required properties:
+- compatible : must be "linn,eth-sniffer"
+- 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"
+- 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
+- fifo-block-words : number of words in one data FIFO entry
+- tstamp-hz : frequency of the timestamp counter
+- tstamp-shift : shift value for the timestamp cyclecounter struct
+- 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>;
+};
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.
 linux	Linux-specific binding
 lsi	LSI Corp. (LSI Logic)
 lltc	Linear Technology Corporation