Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932466AbbENLbE (ORCPT ); Thu, 14 May 2015 07:31:04 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:37167 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750759AbbENLa7 (ORCPT ); Thu, 14 May 2015 07:30:59 -0400 Date: Thu, 14 May 2015 13:30:54 +0200 From: Richard Cochran To: Jonathan Richardson Cc: Arnd Bergmann , Darren Edamura , One Thousand Gnomes , Scott Branden , Pawel Moll , Ian Campbell , Ray Jui , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Rob Herring , bcm-kernel-feedback-list@broadcom.com, Kumar Gala , Mark Rutland , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus Message-ID: <20150514113054.GB1580@localhost.localdomain> References: <1429744923-2007-1-git-send-email-jonathar@broadcom.com> <5543CD73.2030902@broadcom.com> <20150501203004.3add2c95@lxorguk.ukuu.org.uk> <9193036.7UYD2OGJHk@wuerfel> <554D1649.2030106@broadcom.com> <20150513153544.GC2065@localhost.localdomain> <5553AAEA.30503@broadcom.com> <20150513202716.GA15878@localhost.localdomain> <5553DD7D.8090905@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5553DD7D.8090905@broadcom.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3416 Lines: 84 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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/