Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752396AbbEATBp (ORCPT ); Fri, 1 May 2015 15:01:45 -0400 Received: from mail-gw3-out.broadcom.com ([216.31.210.64]:23960 "EHLO mail-gw3-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751455AbbEATBm (ORCPT ); Fri, 1 May 2015 15:01:42 -0400 X-IronPort-AV: E=Sophos;i="5.13,351,1427785200"; d="scan'208";a="63566976" Message-ID: <5543CD73.2030902@broadcom.com> Date: Fri, 1 May 2015 12:01:07 -0700 From: Jonathan Richardson User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Arnd Bergmann CC: Scott Branden , Darren Edamura , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Ray Jui , Greg Kroah-Hartman , , , , Subject: Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus References: <1429744923-2007-1-git-send-email-jonathar@broadcom.com> <1429744923-2007-3-git-send-email-jonathar@broadcom.com> <6346218.3x3hbSlxMs@wuerfel> In-Reply-To: <6346218.3x3hbSlxMs@wuerfel> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8348 Lines: 216 Arnd, some more info is provided and also some questions before I can proceed. On 15-04-23 01:04 AM, Arnd Bergmann wrote: > On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote: >> Reviewed-by: Scott Branden >> Tested-by: Scott Branden >> Signed-off-by: Jonathan Richardson > > No description at all? The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 "clients" -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum dte_client' from bcm_cygnus_dte.h. The DTE timestamper creates timestamps based on the current clock wall time. When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time. The isochronous interrupt is used to pull the timestamps out of the h/w FIFO and store them in a s/w FIFO until they are pulled out of that from user space. The API we provide in the driver enables clients (inputs into the timestamper) using dte_enable_timestamp(). The clients divider is set with dte_set_client_divider(). The isochronous interrupt frequency - the rate at which the ISR fires to pull timestamps from h/w to s/w FIFO is dte_set_irq_interval(). Timestamps generated by the timestamper for clients are polled for using dte_get_timestamp(). The clock is controlled with the remaining functions: dte_set_time(), dte_get_time(), dte_adj_time(), dte_adj_freq(). These functions provide the time base for the timestamper but also provide a clock that was meant to be used later by an Ethernet driver for PTP. The ioctls correspond to all of these functions for the user space API. The idea for future PTP support was to use the dte_get_time() to get timestamps for the packets. This may make the driver more suitable to be PTP driver. It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do). > > You are introducing a new subsystem here, which means you get to put > a lot of thought into the API design, to ensure that it works with > other drivers of the same type, and that we don't already have > a subsystem that does what you need here. > > Please write a few pages of text about the tradeoffs that went into > the internal and user-facing API design, and explain the differences > to the existing infrastructure we have for the clocksource, clockevent, > k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks, > in particular why your hardware cannot fit into the existing frameworks > and has to have a new one. > >> +struct bcm_cygnus_dte *dte_get_dev_from_devname(const char *devname) >> +{ >> + struct bcm_cygnus_dte *cygnus_dte = NULL; >> + bool found = false; >> + >> + if (!devname) >> + return NULL; >> + >> + list_for_each_entry(cygnus_dte, &dtedev_list, node) { >> + if (!strcmp(dev_name(&cygnus_dte->pdev->dev), devname)) { >> + /* Matched on device name */ >> + found = true; >> + break; >> + } >> + } >> + >> + return found ? cygnus_dte : NULL; >> +} >> +EXPORT_SYMBOL(dte_get_dev_from_devname); > > No, don't match on a device name. If you must have a reference, use > a phandle in DT. > >> +int dte_get_timestamp( >> + struct bcm_cygnus_dte *cygnus_dte, >> + enum dte_client client, >> + struct timespec *ts) > > Please use timespec64 or ktime_t for internal interfaces. > >> + case DTE_IOCTL_SET_DIVIDER: > > For the IOCTLs, please write a documentation in man-page form. No need > for troff formatting, plain text is fine. Sure, where should this man page live in the kernel? > >> +/** >> + * DTE Client >> + */ >> +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, >> +}; > > Make this more abstract, so we can reuse the API for other vendors. > >> +#define DTE_IOCTL_BASE 'd' >> +#define DTE_IO(nr) _IO(DTE_IOCTL_BASE, nr) >> +#define DTE_IOR(nr, type) _IOR(DTE_IOCTL_BASE, nr, type) >> +#define DTE_IOW(nr, type) _IOW(DTE_IOCTL_BASE, nr, type) >> +#define DTE_IOWR(nr, type) _IOWR(DTE_IOCTL_BASE, nr, type) >> + >> +#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data) >> +#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data) >> +#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data) >> +#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp) >> +#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec) >> +#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec) > > Instead of timespec, use a pair of '__u64' values, or alternatively > just a '__u64' for nanoseconds if the API does not have to cover > times before 1970 or after 2262. > >> +#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t) >> +#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t) > > Maybe 'struct timex' for the adjustment? > > Also, how about adding new syscalls along the lines of > the timerfd stuff instead of using ioctl? It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using clock_gettime(), clock_settime(), clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver from ptp_clock_adjtime(). We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using 'n_ext_ts' in struct ptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver because ptp_ioctl() is the ioctl handler. I don't see how any of the API's currently do what we need other than the flexibility a char device provides. Just wanted to get your thoughts on this before I start looking at an entirely new user and kernel API for DTE. Really what we need is PTP with extended external timestamping channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks. > >> +struct dte_data { >> + enum dte_client client; >> + unsigned int data; >> +}; > > No 'enum' in ioctl data, always use '__u32' etc. > >> + >> +struct dte_timestamp { >> + enum dte_client client; >> + struct timespec ts; >> +}; > > Instead of timespec, use a pair of '__u64' values, or alternatively > just a '__u64' for nanoseconds if the API does not have to cover > times before 1970 or after 2262. > >> +struct bcm_cygnus_dte; >> + >> +extern struct bcm_cygnus_dte *dte_get_dev_from_devname( >> + const char *devname); >> +extern int dte_enable_timestamp( >> + struct bcm_cygnus_dte *cygnus_dte, >> + enum dte_client client, >> + int enable); > > > Put the internal declarations into one header in include/linux, and the > user space facing ones in another one in include/uapi/linux. > > Arnd > -- 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/