linux-kernel - Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <555D1ADA.8070308@broadcom.com>
Date:	Wed, 20 May 2015 16:38:02 -0700
From:	Jonathan Richardson <jonathar@...adcom.com>
To:	Richard Cochran <richardcochran@...il.com>
CC:	Arnd Bergmann <arnd@...db.de>,
	Darren Edamura <dedamura@...adcom.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Scott Branden <sbranden@...adcom.com>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Ray Jui <rjui@...adcom.com>,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	<bcm-kernel-feedback-list@...adcom.com>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver
 for cygnus

On 15-05-14 04:30 AM, Richard Cochran wrote:
> On Wed, May 13, 2015 at 04:25:49PM -0700, Jonathan Richardson wrote:
>> On 15-05-13 01:27 PM, Richard Cochran wrote:
>>> If the ISR is delivering batches of time stamps, then the interrupt
>>> rate aught to be the highest rate that the system can support.
>>
>> It to nanosecond precision so it can be ridiculously quick.
> 
> Your interrupt rate directly controls the worst case delay between the
> time the event occurs and the time the application (or stack) obtains
> the time stamp.  You want this delay to be as short as possible, but
> you cannot afford to bog down the entire operating system.  I would
> suggest a fixed value of 1000 Hz (with perhaps a debugfs knob).
> 
> What a poor hardware design.  Oh well.
> 
>> Having separate FIFO's allows process A to only retrieve channel 1
>> timestamps.
> 
> Having lots of different processes reading time stamps directly, and
> trying to match them up with various HW events after the fact, is not
> way to go.  Instead, kernel time stamp consumers should pair the time
> stamps with the associated events and then provide the time
> information conveniently over the existing APIs. (See also my last
> comment.)
> 
>> Our original non PTP driver ioctl (DTE_GET_TIMESTAMP) solved that
>> problem because we could specify a channel. We're now trying to adapt it
>> to PTP so we don't have to write a new "DTE" user and kernel side API.
> 
> For mainline, we want the best interface possible.  Sometimes that
> means that programs based on out-of-tree interfaces will need to be
> changed.
> 
>>> Ideally, there will be kernel consumers for most of the channels, and
>>> they will forward the time stamps in a way appropriate to their
>>> subsystem.  For example, network devices will use so_timestamping. 
>>> Any "leftover" channels can go through the generic PTP interface.
>>
>> I'll look more into so_timestamping to see how that's used but we wanted
>> one generic interface to get timestamps from channels because anything
>> can be hooked up to a channel.
> 
> I definitely do *not* want one generic interface.  The task of
> matching time stamps with hardware events belongs to the kernel.  For
> random GPIOs, the existing PTP ioctl is plenty good enough, but for
> other devices (network, audio) more work is needed.

Richard, this design isn't going to work. We need to have both kernel
and user space consumers. We don't want all GPIO's in a common timestamp
buffer either, as it presents problems I mentioned previously. Currently
the network input is a gpio. After some discussion here I think we'll
have to keep this driver out of the kernel for now.

Thanks.

> 
> One huge lacuna in your patch series is the code that associates the
> time stamps with events.  How is this supposed to work?
> 
> So far you have:
> 
> +enum dte_client {
> +       DTE_CLIENT_MIN = 0,
> +       DTE_CLIENT_I2S0_BITCLOCK = 0,
> +       DTE_CLIENT_I2S1_BITCLOCK,
> +       DTE_CLIENT_I2S2_BITCLOCK,
> +       DTE_CLIENT_I2S0_WORDCLOCK,
> +       DTE_CLIENT_I2S1_WORDCLOCK,
> +       DTE_CLIENT_I2S2_WORDCLOCK,
> +       DTE_CLIENT_LCD_CLFP,
> +       DTE_CLIENT_LCD_CLLP,
> +       DTE_CLIENT_GPIO14,
> +       DTE_CLIENT_GPIO15,
> +       DTE_CLIENT_GPIO22,
> +       DTE_CLIENT_GPIO23,
> +       DTE_CLIENT_MAX,
> +};
> 
> Network devices are not present at all.  No idea why LCD signals are
> included.  For the i2s, this appears to stamp the audio clock.  But
> which audio sample has been stamped?  Or do you only care about
> frequency and not phase?
> 
> For the next round, please include John Stulz, Thomas Gleixner,
> netdev, and the appropriate audio list on CC.
> 
> Thanks,
> Richard
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/