Re: [PATCH v2 net-next] net: stmmac: Add PPS and Flexible PPS support

[Richard Cochran <richardcochran@gmail.com>]

On Fri, May 25, 2018 at 04:32:52PM +0100, Jose Abreu wrote:
> +int dwmac5_pps_config(void __iomem *ioaddr, bool enable)
> +{
> +	u32 val = readl(ioaddr + MAC_PPS_CONTROL);
> +
> +	/* There is no way to disable fixed PPS output so we just reset
> +	 * the values to make sure its in fixed PPS mode */

In that case, don't try to make this appear to be programmable.  Just
reset the registers in your probe or setup function unconditionally.

> +	val &= ~PPSx_MASK(0);
> +	val |= TRGTMODSELx(0, 0x2);
> +
> +	writel(val, ioaddr + MAC_PPS_CONTROL);
> +	return 0;
> +}
> +
> +int dwmac5_flex_pps_config(void __iomem *ioaddr, int index,
> +			   struct stmmac_pps_cfg *cfg, bool enable,
> +			   u32 sub_second_inc, u32 systime_flags)
> +{
> +	u32 tnsec = readl(ioaddr + MAC_PPSx_TARGET_TIME_NSEC(index));
> +	u32 val = readl(ioaddr + MAC_PPS_CONTROL);
> +	u64 period;
> +
> +	if (!cfg->available)
> +		return -EINVAL;
> +	if (tnsec & TRGTBUSY0)
> +		return -EBUSY;
> +	if (!sub_second_inc || !systime_flags)
> +		return -EINVAL;

Don't add tests on the arguments like this.  Instead, make sure the
caller always provides correct values.

> +
> +	val &= ~PPSx_MASK(index);
> +
> +	if (!enable) {
> +		val |= PPSCMDx(index, 0x5);
> +		writel(val, ioaddr + MAC_PPS_CONTROL);
> +		return 0;
> +	}
> +
> +	val |= PPSCMDx(index, 0x2);
> +	val |= TRGTMODSELx(index, 0x2);
> +	val |= PPSEN0;
> +
> +	writel(cfg->start.tv_sec, ioaddr + MAC_PPSx_TARGET_TIME_SEC(index));
> +
> +	if (!(systime_flags & PTP_TCR_TSCTRLSSR))
> +		cfg->start.tv_nsec = (cfg->start.tv_nsec * 1000) / 465;
> +	writel(cfg->start.tv_nsec, ioaddr + MAC_PPSx_TARGET_TIME_NSEC(index));
> +
> +	period = cfg->period.tv_sec * 1000000000;
> +	period += cfg->period.tv_nsec;

> +	struct timespec64 period;

> +
> +	do_div(period, sub_second_inc);
> +
> +	if (period <= 1)
> +		return -EINVAL;
> +
> +	writel(period - 1, ioaddr + MAC_PPSx_INTERVAL(index));
> +
> +	period >>= 1;
> +	if (period <= 1)
> +		return -EINVAL;
> +
> +	writel(period - 1, ioaddr + MAC_PPSx_WIDTH(index));
> +
> +	/* Finally, activate it */
> +	writel(val, ioaddr + MAC_PPS_CONTROL);
> +	return 0;
> +}

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> index 7d3a5c7..35c6d0c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ptp.c
> @@ -140,19 +140,50 @@ static int stmmac_set_time(struct ptp_clock_info *ptp,
>  static int stmmac_enable(struct ptp_clock_info *ptp,
>  			 struct ptp_clock_request *rq, int on)
>  {
> -	return -EOPNOTSUPP;
> +	struct stmmac_priv *priv =
> +	    container_of(ptp, struct stmmac_priv, ptp_clock_ops);
> +	struct stmmac_pps_cfg *cfg;
> +	int ret = -EOPNOTSUPP;
> +	unsigned long flags;
> +
> +	switch (rq->type) {
> +	case PTP_CLK_REQ_PEROUT:
> +		cfg = &priv->pps[rq->perout.index];
> +
> +		cfg->start.tv_sec = rq->perout.start.sec;
> +		cfg->start.tv_nsec = rq->perout.start.nsec;
> +		cfg->period.tv_sec = rq->perout.period.sec;
> +		cfg->period.tv_nsec = rq->perout.period.nsec;
> +
> +		spin_lock_irqsave(&priv->ptp_lock, flags);
> +		ret = stmmac_flex_pps_config(priv, priv->ioaddr,
> +					     rq->perout.index, cfg, on,
> +					     priv->sub_second_inc,
> +					     priv->systime_flags);
> +		spin_unlock_irqrestore(&priv->ptp_lock, flags);
> +		break;
> +	case PTP_CLK_REQ_PPS:
> +		spin_lock_irqsave(&priv->ptp_lock, flags);
> +		ret = stmmac_pps_config(priv, priv->ioaddr, on);

This is not what PTP_CLK_REQ_PPS is for.  It only to arrange for a
trigger into the kernel's PPS sub-system.

[ Sorry that the ptp header files don't explain this in the comments.
  I should really fix that. ]

> +		spin_unlock_irqrestore(&priv->ptp_lock, flags);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
>  }
>  
>  /* structure describing a PTP hardware clock */
> -static const struct ptp_clock_info stmmac_ptp_clock_ops = {
> +static struct ptp_clock_info stmmac_ptp_clock_ops = {
>  	.owner = THIS_MODULE,
>  	.name = "stmmac_ptp_clock",
>  	.max_adj = 62500000,
>  	.n_alarm = 0,
>  	.n_ext_ts = 0,
> -	.n_per_out = 0,
> +	.n_per_out = 0, /* will be overwritten in stmmac_ptp_register */
>  	.n_pins = 0,
> -	.pps = 0,
> +	.pps = 0, /* will be overwritten in stmmac_ptp_register */

Again, this is for inputting a PPS event into the kernel's PPS subsystem.

Thanks,
Richard