Date: Mon, 11 Dec 2017 14:59:54 -0800
From: Richard Cochran <richardcochran@gmail.com>
To: Aleksey Makarov <aleksey.makarov@cavium.com>
Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org,
        "Goutham, Sunil" <Sunil.Goutham@cavium.com>,
        Radoslaw Biernacki <rad@semihalf.com>,
        Robert Richter <rric@kernel.org>,
        David Daney <ddaney@caviumnetworks.com>,
        Philippe Ombredanne <pombredanne@nexb.com>
Subject: Re: [PATCH net-next v5 1/2] net: add support for Cavium PTP
 coprocessor
Message-ID: <20171211225954.ezqut6jvfg65rg4w@localhost>
References: <20171211141435.2915-1-aleksey.makarov@cavium.com>
 <20171211141435.2915-2-aleksey.makarov@cavium.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171211141435.2915-2-aleksey.makarov@cavium.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3335
Lines: 123


Sorry I didn't finish reviewing before...

On Mon, Dec 11, 2017 at 05:14:30PM +0300, Aleksey Makarov wrote:
> +/**
> + * cavium_ptp_adjfreq() - Adjust ptp frequency
> + * @ptp: PTP clock info
> + * @ppb: how much to adjust by, in parts-per-billion
> + */
> +static int cavium_ptp_adjfreq(struct ptp_clock_info *ptp_info, s32 ppb)

adjfreq() is deprecated.  See ptp_clock_kernel.h.  Please re-work this
to implement the adjfine() method instead.

> +/**
> + * cavium_ptp_enable() - Check if PTP is enabled

Nit - comment is not correct. This method is for the auxiliary PHC
functions.

> + * @ptp: PTP clock info
> + * @rq:  request
> + * @on:  is it on
> + */
> +static int cavium_ptp_enable(struct ptp_clock_info *ptp_info,
> +			     struct ptp_clock_request *rq, int on)
> +{
> +	return -EOPNOTSUPP;
> +}

...

> +static int cavium_ptp_probe(struct pci_dev *pdev,
> +			    const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cavium_ptp *clock;
> +	struct cyclecounter *cc;
> +	u64 clock_cfg;
> +	u64 clock_comp;
> +	int err;
> +
> +	clock = devm_kzalloc(dev, sizeof(*clock), GFP_KERNEL);
> +	if (!clock)
> +		return -ENOMEM;
> +
> +	clock->pdev = pdev;
> +
> +	err = pcim_enable_device(pdev);
> +	if (err)
> +		return err;
> +
> +	err = pcim_iomap_regions(pdev, 1 << PCI_PTP_BAR_NO, pci_name(pdev));
> +	if (err)
> +		return err;
> +
> +	clock->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO];
> +
> +	spin_lock_init(&clock->spin_lock);
> +
> +	cc = &clock->cycle_counter;
> +	cc->read = cavium_ptp_cc_read;
> +	cc->mask = CYCLECOUNTER_MASK(64);
> +	cc->mult = 1;
> +	cc->shift = 0;
> +
> +	timecounter_init(&clock->time_counter, &clock->cycle_counter,
> +			 ktime_to_ns(ktime_get_real()));
> +
> +	clock->clock_rate = ptp_cavium_clock_get();
> +
> +	clock->ptp_info = (struct ptp_clock_info) {
> +		.owner		= THIS_MODULE,
> +		.name		= "ThunderX PTP",
> +		.max_adj	= 1000000000ull,
> +		.n_ext_ts	= 0,
> +		.n_pins		= 0,
> +		.pps		= 0,
> +		.adjfreq	= cavium_ptp_adjfreq,
> +		.adjtime	= cavium_ptp_adjtime,
> +		.gettime64	= cavium_ptp_gettime,
> +		.settime64	= cavium_ptp_settime,
> +		.enable		= cavium_ptp_enable,
> +	};
> +
> +	clock_cfg = readq(clock->reg_base + PTP_CLOCK_CFG);
> +	clock_cfg |= PTP_CLOCK_CFG_PTP_EN;
> +	writeq(clock_cfg, clock->reg_base + PTP_CLOCK_CFG);
> +
> +	clock_comp = ((u64)1000000000ull << 32) / clock->clock_rate;
> +	writeq(clock_comp, clock->reg_base + PTP_CLOCK_COMP);
> +
> +	clock->ptp_clock = ptp_clock_register(&clock->ptp_info, dev);
> +	if (IS_ERR(clock->ptp_clock)) {

You need to handle the case when ptp_clock_register() returns NULL.

from ptp_clock_kernel.h:

/**
 * ptp_clock_register() - register a PTP hardware clock driver
 *
 * @info:   Structure describing the new clock.
 * @parent: Pointer to the parent device of the new clock.
 *
 * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
 * support is missing at the configuration level, this function
 * returns NULL, and drivers are expected to gracefully handle that
 * case separately.
 */

> +		clock_cfg = readq(clock->reg_base + PTP_CLOCK_CFG);
> +		clock_cfg &= ~PTP_CLOCK_CFG_PTP_EN;
> +		writeq(clock_cfg, clock->reg_base + PTP_CLOCK_CFG);
> +		return PTR_ERR(clock->ptp_clock);
> +	}
> +
> +	pci_set_drvdata(pdev, clock);
> +	return 0;
> +}

Thanks,
Richard