Message-ID: <20120310110230.GB3489@netboy.at.omicron.at>
Date:	Sat, 10 Mar 2012 12:02:32 +0100
From:	Richard Cochran <richardcochran@...il.com>
To:	Takahiro Shimizu <tshimizu818@...il.com>
Cc:	jeffrey.t.kirsher@...el.com, davem@...emloft.net,
	lucas.demarchi@...fusion.mobi, mirq-linux@...e.qmqm.pl,
	paul.gortmaker@...driver.com, jdmason@...zu.us,
	john.stultz@...aro.org, arnd@...db.de, khc@...waw.pl,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	qi.wang@...el.com, yong.y.wang@...el.com, joel.clark@...el.com,
	kok.howg.ewe@...el.com
Subject: Re: [PATCH RE-SUBMIT 1/2] supports eg20t ptp clock

This looks good, for the most part.  However, I did find a few issues
though, below.

Thanks,
Richard

On Thu, Mar 08, 2012 at 05:16:26PM +0900, Takahiro Shimizu wrote:
> Supports EG20T ptp clock in the driver
> 
> Changes e-mail address.
> 
> Adds number.
> 
> Signed-off-by: Takahiro Shimizu <tshimizu818@...il.com>
> ---
>  drivers/ptp/Kconfig   |   13 +
>  drivers/ptp/Makefile  |    1 +
>  drivers/ptp/ptp_pch.c |  731 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 745 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/ptp/ptp_pch.c
> 
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 68d7201..cd9bc3b 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -72,4 +72,17 @@ config DP83640_PHY
>  	  In order for this to work, your MAC driver must also
>  	  implement the skb_tx_timetamp() function.
>  
> +config PTP_1588_CLOCK_PCH
> +	tristate "Intel PCH EG20T as PTP clock"
> +	depends on PTP_1588_CLOCK
> +	depends on PCH_GBE
> +	help
> +	  This driver adds support for using the PCH EG20T as a PTP
> +	  clock. This clock is only useful if your PTP programs are
> +	  getting hardware time stamps on the PTP Ethernet packets
> +	  using the SO_TIMESTAMPING API.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ptp_pch.
> +
>  endmenu
> diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
> index f6933e8..8b58597 100644
> --- a/drivers/ptp/Makefile
> +++ b/drivers/ptp/Makefile
> @@ -5,3 +5,4 @@
>  ptp-y					:= ptp_clock.o ptp_chardev.o ptp_sysfs.o
>  obj-$(CONFIG_PTP_1588_CLOCK)		+= ptp.o
>  obj-$(CONFIG_PTP_1588_CLOCK_IXP46X)	+= ptp_ixp46x.o
> +obj-$(CONFIG_PTP_1588_CLOCK_PCH)	+= ptp_pch.o
> diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
> new file mode 100644
> index 0000000..1a06e02
> --- /dev/null
> +++ b/drivers/ptp/ptp_pch.c
> @@ -0,0 +1,731 @@
> +/*
> + * PTP 1588 clock using the EG20T PCH
> + *
> + * Copyright (C) 2010 OMICRON electronics GmbH
> + * Copyright (C) 2011-2012 LAPIS SEMICONDUCTOR Co., LTD.
> + *
> + * This code was derived from the IXP46X driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/ptp_clock_kernel.h>
> +
> +#define STATION_ADDR_LEN	20
> +#define PCI_DEVICE_ID_PCH_1588	0x8819
> +#define IO_MEM_BAR 1
> +
> +#define DEFAULT_ADDEND 0xA0000000

The frequency adjustment problems that you reported on linuxptp-users
would be caused by wrong values of DEFAULT_ADDEND and TICKS_NS_SHIFT.
What is the input clock frequency for this hardware?

> +#define TICKS_NS_SHIFT  5

You have defined this TICKS_NS_SHIFT twice, once here, and once in
pch_gbe_main.c in the following patch. You don't need to use this
macro in pch_gbe_main.c at all. See below.

[ Even if you did need this macro in two different .c files, then the
  right way would be to use a header file. BTW the PHC driver does not
  necessarily have to go into drivers/ptp. If it works better to keep
  the PHC driver in the same directory as the MAC driver, to use a
  shared header for example, then that is fine, too. See gianfar for
  an example of that. ]

> +#define N_EXT_TS	2
> +
> +enum pch_status {
> +	PCH_SUCCESS,
> +	PCH_INVALIDPARAM,
> +	PCH_NOTIMESTAMP,
> +	PCH_INTERRUPTMODEINUSE,
> +	PCH_FAILED,
> +	PCH_UNSUPPORTED,
> +};
> +/**
> + * struct pch_ts_regs - IEEE 1588 registers
> + */
> +struct pch_ts_regs {
> +	u32 control;
> +	u32 event;
> +	u32 addend;
> +	u32 accum;
> +	u32 test;
> +	u32 ts_compare;
> +	u32 rsystime_lo;
> +	u32 rsystime_hi;
> +	u32 systime_lo;
> +	u32 systime_hi;
> +	u32 trgt_lo;
> +	u32 trgt_hi;
> +	u32 asms_lo;
> +	u32 asms_hi;
> +	u32 amms_lo;
> +	u32 amms_hi;
> +	u32 ch_control;
> +	u32 ch_event;
> +	u32 tx_snap_lo;
> +	u32 tx_snap_hi;
> +	u32 rx_snap_lo;
> +	u32 rx_snap_hi;
> +	u32 src_uuid_lo;
> +	u32 src_uuid_hi;
> +	u32 can_status;
> +	u32 can_snap_lo;
> +	u32 can_snap_hi;
> +	u32 ts_sel;
> +	u32 ts_st[6];
> +	u32 reserve1[14];
> +	u32 stl_max_set_en;
> +	u32 stl_max_set;
> +	u32 reserve2[13];
> +	u32 srst;
> +};
> +
> +#define PCH_TSC_RESET		(1 << 0)
> +#define PCH_TSC_TTM_MASK	(1 << 1)
> +#define PCH_TSC_ASMS_MASK	(1 << 2)
> +#define PCH_TSC_AMMS_MASK	(1 << 3)
> +#define PCH_TSC_PPSM_MASK	(1 << 4)
> +#define PCH_TSE_TTIPEND		(1 << 1)
> +#define PCH_TSE_SNS		(1 << 2)
> +#define PCH_TSE_SNM		(1 << 3)
> +#define PCH_TSE_PPS		(1 << 4)
> +#define PCH_CC_MM		(1 << 0)
> +#define PCH_CC_TA		(1 << 1)
> +
> +#define PCH_CC_MODE_SHIFT	16
> +#define PCH_CC_MODE_MASK	0x001F0000
> +#define PCH_CC_VERSION		(1 << 31)
> +#define PCH_CE_TXS		(1 << 0)
> +#define PCH_CE_RXS		(1 << 1)
> +#define PCH_CE_OVR		(1 << 0)
> +#define PCH_CE_VAL		(1 << 1)
> +#define PCH_ECS_ETH		(1 << 0)
> +
> +#define PCH_ECS_CAN		(1 << 1)
> +#define PCH_STATION_BYTES	6
> +
> +#define PCH_IEEE1588_ETH	(1 << 0)
> +#define PCH_IEEE1588_CAN	(1 << 1)
> +/**
> + * struct pch_dev - Driver private data
> + */
> +struct pch_dev {
> +	struct pch_ts_regs *regs;
> +	struct ptp_clock *ptp_clock;
> +	struct ptp_clock_info caps;
> +	int exts0_enabled;
> +	int exts1_enabled;
> +
> +	u32 mem_base;
> +	u32 mem_size;
> +	u32 irq;
> +	struct pci_dev *pdev;
> +	spinlock_t register_lock;
> +};
> +
> +/**
> + * struct pch_params - 1588 module parameter
> + */
> +struct pch_params {
> +	u8 station[STATION_ADDR_LEN];
> +};
> +
> +/* structure to hold the module parameters */
> +static struct pch_params pch_param = {
> +	"00:00:00:00:00:00"
> +};
> +
> +/*
> + * Register access functions
> + */
> +static inline void pch_eth_enable_set(struct pch_dev *chip)
> +{
> +	u32 val;
> +	/* SET the eth_enable bit */
> +	val = ioread32(&chip->regs->ts_sel) | (PCH_ECS_ETH);
> +	iowrite32(val, (&chip->regs->ts_sel));
> +}
> +
> +static u64 pch_systime_read(struct pch_ts_regs *regs)
> +{
> +	u64 ns;
> +	u32 lo, hi;
> +
> +	lo = ioread32(&regs->systime_lo);
> +	hi = ioread32(&regs->systime_hi);
> +
> +	ns = ((u64) hi) << 32;
> +	ns |= lo;
> +	ns <<= TICKS_NS_SHIFT;
> +
> +	return ns;
> +}
> +
> +static void pch_systime_write(struct pch_ts_regs *regs, u64 ns)
> +{
> +	u32 hi, lo;
> +
> +	ns >>= TICKS_NS_SHIFT;
> +	hi = ns >> 32;
> +	lo = ns & 0xffffffff;
> +
> +	iowrite32(lo, &regs->systime_lo);
> +	iowrite32(hi, &regs->systime_hi);
> +}
> +
> +static inline void pch_block_reset(struct pch_dev *chip)
> +{
> +	u32 val;
> +	/* Reset Hardware Assist block */
> +	val = ioread32(&chip->regs->control) | PCH_TSC_RESET;
> +	iowrite32(val, (&chip->regs->control));
> +	val = val & ~PCH_TSC_RESET;
> +	iowrite32(val, (&chip->regs->control));
> +}
> +
> +u32 pch_ch_control_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u32 val;
> +
> +	val = ioread32(&chip->regs->ch_control);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(pch_ch_control_read);
> +
> +void pch_ch_control_write(struct pci_dev *pdev, u32 val)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> +	iowrite32(val, (&chip->regs->ch_control));
> +}
> +EXPORT_SYMBOL(pch_ch_control_write);
> +
> +u32 pch_ch_event_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u32 val;
> +
> +	val = ioread32(&chip->regs->ch_event);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(pch_ch_event_read);
> +
> +void pch_ch_event_write(struct pci_dev *pdev, u32 val)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> +	iowrite32(val, (&chip->regs->ch_event));
> +}
> +EXPORT_SYMBOL(pch_ch_event_write);
> +
> +u32 pch_src_uuid_lo_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u32 val;
> +
> +	val = ioread32(&chip->regs->src_uuid_lo);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(pch_src_uuid_lo_read);
> +
> +u32 pch_src_uuid_hi_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u32 val;
> +
> +	val = ioread32(&chip->regs->src_uuid_hi);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(pch_src_uuid_hi_read);
> +
> +u64 pch_rx_snap_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u64 ns;
> +	u32 lo, hi;
> +
> +	lo = ioread32(&chip->regs->rx_snap_lo);
> +	hi = ioread32(&chip->regs->rx_snap_hi);
> +
> +	ns = ((u64) hi) << 32;
> +	ns |= lo;

Here you should correct the 'ns' using the TICKS_NS_SHIFT so that the
return value is in nanoseconds.

> +
> +	return ns;
> +}
> +EXPORT_SYMBOL(pch_rx_snap_read);
> +
> +u64 pch_tx_snap_read(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +	u64 ns;
> +	u32 lo, hi;
> +
> +	lo = ioread32(&chip->regs->tx_snap_lo);
> +	hi = ioread32(&chip->regs->tx_snap_hi);
> +
> +	ns = ((u64) hi) << 32;
> +	ns |= lo;

Here again, make the return value be in nanoseconds. Then you can
remove TICKS_NS_SHIFT from pch_gbe_main.c altogether.

> +
> +	return ns;
> +}
> +EXPORT_SYMBOL(pch_tx_snap_read);
> +
> +/* This function enables all 64 bits in system time registers [high & low].
> +This is a work-around for non continuous value in the SystemTime Register*/
> +static void pch_set_system_time_count(struct pch_dev *chip)
> +{
> +	iowrite32(0x01, &chip->regs->stl_max_set_en);
> +	iowrite32(0xFFFFFFFF, &chip->regs->stl_max_set);
> +	iowrite32(0x00, &chip->regs->stl_max_set_en);
> +}
> +
> +static void pch_reset(struct pch_dev *chip)
> +{
> +	/* Reset Hardware Assist */
> +	pch_block_reset(chip);
> +
> +	/* enable all 32 bits in system time registers */
> +	pch_set_system_time_count(chip);
> +}
> +
> +/**
> + * pch_set_station_address() - This API sets the station address used by
> + *				    IEEE 1588 hardware when looking at PTP
> + *				    traffic on the  ethernet interface
> + * @addr:	dress which contain the column separated address to be used.
> + */
> +static int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
> +{
> +	s32 i;
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> +	/* Verify the parameter */
> +	if ((chip->regs == 0) || addr == (u8 *)NULL) {
> +		dev_err(&pdev->dev,
> +			"invalid params returning PCH_INVALIDPARAM\n");
> +		return PCH_INVALIDPARAM;
> +	}
> +	/* For all station address bytes */
> +	for (i = 0; i < PCH_STATION_BYTES; i++) {
> +		u32 val;
> +		s32 tmp;
> +
> +		tmp = hex_to_bin(addr[i * 3]);
> +		if (tmp < 0) {
> +			dev_err(&pdev->dev,
> +				"invalid params returning PCH_INVALIDPARAM\n");
> +			return PCH_INVALIDPARAM;
> +		}
> +		val = tmp * 16;
> +		tmp = hex_to_bin(addr[(i * 3) + 1]);
> +		if (tmp < 0) {
> +			dev_err(&pdev->dev,
> +				"invalid params returning PCH_INVALIDPARAM\n");
> +			return PCH_INVALIDPARAM;
> +		}
> +		val += tmp;
> +		/* Expects ':' separated addresses */
> +		if ((i < 5) && (addr[(i * 3) + 2] != ':')) {
> +			dev_err(&pdev->dev,
> +				"invalid params returning PCH_INVALIDPARAM\n");
> +			return PCH_INVALIDPARAM;
> +		}
> +
> +		/* Ideally we should set the address only after validating
> +							 entire string */
> +		dev_dbg(&pdev->dev, "invoking pch_station_set\n");
> +		iowrite32(val, &chip->regs->ts_st[i]);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Interrupt service routine
> + */
> +static irqreturn_t isr(int irq, void *priv)
> +{
> +	struct pch_dev *pch_dev = priv;
> +	struct pch_ts_regs *regs = pch_dev->regs;
> +	struct ptp_clock_event event;
> +	u32 ack = 0, lo, hi, val;
> +
> +	val = ioread32(&regs->event);
> +
> +	if (val & PCH_TSE_SNS) {
> +		ack |= PCH_TSE_SNS;
> +		if (pch_dev->exts0_enabled) {
> +			hi = ioread32(&regs->asms_hi);
> +			lo = ioread32(&regs->asms_lo);
> +			event.type = PTP_CLOCK_EXTTS;
> +			event.index = 0;
> +			event.timestamp = ((u64) hi) << 32;
> +			event.timestamp |= lo;
> +			event.timestamp <<= TICKS_NS_SHIFT;
> +			ptp_clock_event(pch_dev->ptp_clock, &event);
> +		}
> +	}
> +
> +	if (val & PCH_TSE_SNM) {
> +		ack |= PCH_TSE_SNM;
> +		if (pch_dev->exts1_enabled) {
> +			hi = ioread32(&regs->amms_hi);
> +			lo = ioread32(&regs->amms_lo);
> +			event.type = PTP_CLOCK_EXTTS;
> +			event.index = 1;
> +			event.timestamp = ((u64) hi) << 32;
> +			event.timestamp |= lo;
> +			event.timestamp <<= TICKS_NS_SHIFT;
> +			ptp_clock_event(pch_dev->ptp_clock, &event);
> +		}
> +	}
> +
> +	if (val & PCH_TSE_TTIPEND)
> +		ack |= PCH_TSE_TTIPEND; /* this bit seems to be always set */
> +
> +	if (ack) {
> +		iowrite32(ack, &regs->event);
> +		return IRQ_HANDLED;
> +	} else
> +		return IRQ_NONE;
> +}
> +
> +/*
> + * PTP clock operations
> + */
> +
> +static int ptp_pch_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	u64 adj;
> +	u32 diff, addend;
> +	int neg_adj = 0;
> +	struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +	struct pch_ts_regs *regs = pch_dev->regs;
> +
> +	if (ppb < 0) {
> +		neg_adj = 1;
> +		ppb = -ppb;
> +	}
> +	addend = DEFAULT_ADDEND;
> +	adj = addend;
> +	adj *= ppb;
> +	diff = div_u64(adj, 1000000000ULL);
> +
> +	addend = neg_adj ? addend - diff : addend + diff;
> +
> +	iowrite32(addend, &regs->addend);
> +
> +	return 0;
> +}
> +
> +static int ptp_pch_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	s64 now;
> +	unsigned long flags;
> +	struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +	struct pch_ts_regs *regs = pch_dev->regs;
> +
> +	spin_lock_irqsave(&pch_dev->register_lock, flags);
> +	now = pch_systime_read(regs);
> +	now += delta;
> +	pch_systime_write(regs, now);
> +	spin_unlock_irqrestore(&pch_dev->register_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ptp_pch_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> +	u64 ns;
> +	u32 remainder;
> +	unsigned long flags;
> +	struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +	struct pch_ts_regs *regs = pch_dev->regs;
> +
> +	spin_lock_irqsave(&pch_dev->register_lock, flags);
> +	ns = pch_systime_read(regs);
> +	spin_unlock_irqrestore(&pch_dev->register_lock, flags);
> +
> +	ts->tv_sec = div_u64_rem(ns, 1000000000, &remainder);
> +	ts->tv_nsec = remainder;
> +	return 0;
> +}
> +
> +static int ptp_pch_settime(struct ptp_clock_info *ptp,
> +			   const struct timespec *ts)
> +{
> +	u64 ns;
> +	unsigned long flags;
> +	struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +	struct pch_ts_regs *regs = pch_dev->regs;
> +
> +	ns = ts->tv_sec * 1000000000ULL;
> +	ns += ts->tv_nsec;
> +
> +	spin_lock_irqsave(&pch_dev->register_lock, flags);
> +	pch_systime_write(regs, ns);
> +	spin_unlock_irqrestore(&pch_dev->register_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ptp_pch_enable(struct ptp_clock_info *ptp,
> +			  struct ptp_clock_request *rq, int on)
> +{
> +	struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +
> +	switch (rq->type) {
> +	case PTP_CLK_REQ_EXTTS:
> +		switch (rq->extts.index) {
> +		case 0:
> +			pch_dev->exts0_enabled = on ? 1 : 0;
> +			break;
> +		case 1:
> +			pch_dev->exts1_enabled = on ? 1 : 0;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info ptp_pch_caps = {
> +	.owner		= THIS_MODULE,
> +	.name		= "PCH timer",
> +	.max_adj	= 50000000,
> +	.n_ext_ts	= N_EXT_TS,
> +	.pps		= 0,
> +	.adjfreq	= ptp_pch_adjfreq,
> +	.adjtime	= ptp_pch_adjtime,
> +	.gettime	= ptp_pch_gettime,
> +	.settime	= ptp_pch_settime,
> +	.enable		= ptp_pch_enable,
> +};
> +
> +
> +#ifdef CONFIG_PM
> +static s32 pch_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	pci_disable_device(pdev);
> +	pci_enable_wake(pdev, PCI_D3hot, 0);
> +
> +	if (pci_save_state(pdev) != 0) {
> +		dev_err(&pdev->dev, "could not save PCI config state\n");
> +		return -ENOMEM;
> +	}
> +	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
> +	return 0;
> +}
> +
> +static s32 pch_resume(struct pci_dev *pdev)
> +{
> +	s32 ret;
> +
> +	pci_set_power_state(pdev, PCI_D0);
> +	pci_restore_state(pdev);
> +	ret = pci_enable_device(pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "pci_enable_device failed\n");
> +		return ret;
> +	}
> +	pci_enable_wake(pdev, PCI_D3hot, 0);
> +	return 0;
> +}
> +#else
> +#define pch_suspend NULL
> +#define pch_resume NULL
> +#endif
> +
> +static void __devexit pch_remove(struct pci_dev *pdev)
> +{
> +	struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> +	ptp_clock_unregister(chip->ptp_clock);
> +	/* free the interrupt */
> +	if (pdev->irq != 0)
> +		free_irq(pdev->irq, chip);
> +
> +	/* unmap the virtual IO memory space */
> +	if (chip->regs != 0) {
> +		iounmap(chip->regs);
> +		chip->regs = 0;
> +	}
> +	/* release the reserved IO memory space */
> +	if (chip->mem_base != 0) {
> +		release_mem_region(chip->mem_base, chip->mem_size);
> +		chip->mem_base = 0;
> +	}
> +	pci_disable_device(pdev);
> +	kfree(chip);
> +	dev_info(&pdev->dev, "complete\n");
> +}
> +
> +static s32 __devinit
> +pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	s32 ret;
> +	unsigned long flags;
> +	struct pch_dev *chip;
> +
> +	chip = kzalloc(sizeof(struct pch_dev), GFP_KERNEL);
> +	if (chip == NULL)
> +		return -ENOMEM;
> +
> +	/* enable the 1588 pci device */
> +	ret = pci_enable_device(pdev);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "could not enable the pci device\n");
> +		goto err_pci_en;
> +	}
> +
> +	chip->mem_base = pci_resource_start(pdev, IO_MEM_BAR);
> +	if (!chip->mem_base) {
> +		dev_err(&pdev->dev, "could not locate IO memory address\n");
> +		ret = -ENODEV;
> +		goto err_pci_start;
> +	}
> +
> +	/* retrieve the available length of the IO memory space */
> +	chip->mem_size = pci_resource_len(pdev, IO_MEM_BAR);
> +
> +	/* allocate the memory for the device registers */
> +	if (!request_mem_region(chip->mem_base, chip->mem_size, "1588_regs")) {
> +		dev_err(&pdev->dev,
> +			"could not allocate register memory space\n");
> +		ret = -EBUSY;
> +		goto err_req_mem_region;
> +	}
> +
> +	/* get the virtual address to the 1588 registers */
> +	chip->regs = ioremap(chip->mem_base, chip->mem_size);
> +
> +	if (!chip->regs) {
> +		dev_err(&pdev->dev, "Could not get virtual address\n");
> +		ret = -ENOMEM;
> +		goto err_ioremap;
> +	}
> +
> +	chip->caps = ptp_pch_caps;
> +	chip->ptp_clock = ptp_clock_register(&chip->caps);
> +
> +	if (IS_ERR(chip->ptp_clock))
> +		return PTR_ERR(chip->ptp_clock);
> +
> +	spin_lock_init(&chip->register_lock);
> +
> +	ret = request_irq(pdev->irq, &isr, IRQF_SHARED, KBUILD_MODNAME, chip);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "failed to get irq %d\n", pdev->irq);
> +		goto err_req_irq;
> +	}
> +
> +	/* indicate success */
> +	chip->irq = pdev->irq;
> +	chip->pdev = pdev;
> +	pci_set_drvdata(pdev, chip);
> +
> +	spin_lock_irqsave(&chip->register_lock, flags);
> +	/* reset the ieee1588 h/w */
> +	pch_reset(chip);
> +
> +	iowrite32(DEFAULT_ADDEND, &chip->regs->addend);
> +	iowrite32(1, &chip->regs->trgt_lo);
> +	iowrite32(0, &chip->regs->trgt_hi);
> +	iowrite32(PCH_TSE_TTIPEND, &chip->regs->event);
> +	/* Version: IEEE1588 v1 and IEEE1588-2008,  Mode: All Evwnt, Locked  */
> +	iowrite32(0x80020000, &chip->regs->ch_control);

Don't use a magic number. Instead, use the nice macros

	#define V2_MODE       (1<<31)
	#define CAP_MODE2     (1<<17)

from the next patch.

> +
> +	pch_eth_enable_set(chip);
> +
> +	if (strcmp(pch_param.station, "00:00:00:00:00:00") != 0) {
> +		if (pch_set_station_address(pch_param.station, pdev) != 0) {
> +			dev_err(&pdev->dev,
> +			"Invalid station address parameter\n"
> +			"Module loaded but station address not set correctly\n"
> +			);
> +		}
> +	}
> +	spin_unlock_irqrestore(&chip->register_lock, flags);
> +	return 0;
> +
> +err_req_irq:
> +	ptp_clock_unregister(chip->ptp_clock);
> +	iounmap(chip->regs);
> +	chip->regs = 0;
> +
> +err_ioremap:
> +	release_mem_region(chip->mem_base, chip->mem_size);
> +
> +err_req_mem_region:
> +	chip->mem_base = 0;
> +
> +err_pci_start:
> +	pci_disable_device(pdev);
> +
> +err_pci_en:
> +	kfree(chip);
> +	dev_err(&pdev->dev, "probe failed(ret=0x%x)\n", ret);
> +
> +	return ret;
> +}
> +
> +static DEFINE_PCI_DEVICE_TABLE(pch_ieee1588_pcidev_id) = {
> +	{
> +	  .vendor = PCI_VENDOR_ID_INTEL,
> +	  .device = PCI_DEVICE_ID_PCH_1588
> +	 },
> +	{0}
> +};
> +
> +static struct pci_driver pch_pcidev = {
> +	.name = KBUILD_MODNAME,
> +	.id_table = pch_ieee1588_pcidev_id,
> +	.probe = pch_probe,
> +	.remove = pch_remove,
> +	.suspend = pch_suspend,
> +	.resume = pch_resume,
> +};
> +
> +static void __exit ptp_pch_exit(void)
> +{
> +	pci_unregister_driver(&pch_pcidev);
> +}
> +
> +static s32 __init ptp_pch_init(void)
> +{
> +	s32 ret;
> +
> +	/* register the driver with the pci core */
> +	ret = pci_register_driver(&pch_pcidev);
> +
> +	return ret;
> +}
> +
> +module_init(ptp_pch_init);
> +module_exit(ptp_pch_exit);
> +
> +module_param_string(station, pch_param.station, sizeof pch_param.station, 0444);
> +MODULE_PARM_DESC(station,
> +	 "IEEE 1588 station address to use - column separated hex values");
> +
> +MODULE_AUTHOR("LAPIS SEMICONDUCTOR, <tshimizu818@...il.com>");
> +MODULE_DESCRIPTION("PTP clock using the EG20T timer");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 1.7.4.4
> 
--
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/

Powered by blists - more mailing lists