Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Nov 2022 00:01:24 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Arun Ramadoss <arun.ramadoss@microchip.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        woojung.huh@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux@armlinux.org.uk,
        Tristram.Ha@microchip.com, richardcochran@gmail.com
Subject: Re: [RFC Patch net-next v2 2/8] net: dsa: microchip: adding the
 posix clock support
Message-ID: <20221121220124.nictbhbw44ynnemg@skbuf>
References: <20221121154150.9573-1-arun.ramadoss@microchip.com>
 <20221121154150.9573-3-arun.ramadoss@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221121154150.9573-3-arun.ramadoss@microchip.com>
Precedence: bulk

On Mon, Nov 21, 2022 at 09:11:44PM +0530, Arun Ramadoss wrote:
> @@ -17,10 +18,21 @@ config NET_DSA_MICROCHIP_KSZ9477_I2C
>  config NET_DSA_MICROCHIP_KSZ_SPI
>  	tristate "KSZ series SPI connected switch driver"
>  	depends on NET_DSA_MICROCHIP_KSZ_COMMON && SPI
> +	depends on PTP_1588_CLOCK_OPTIONAL
>  	select REGMAP_SPI
>  	help
>  	  Select to enable support for registering switches configured through SPI.
>  
> +config NET_DSA_MICROCHIP_KSZ_PTP
> +	bool "Support for the PTP clock on the KSZ9563/LAN937x Ethernet Switch"
> +	depends on NET_DSA_MICROCHIP_KSZ_COMMON && PTP_1588_CLOCK
> +	help
> +	  This enables support for timestamping & PTP clock manipulation

Please use "and" instead of "&".

> +	  in the KSZ9563/LAN937x Ethernet switch
> +
> +	  Select to enable support for PTP feature for KSZ9563/lan937x series

Please capitalize both KSZ9563 and LAN937X. This help text is the
business card of the feature, you need it to look nice and shiny.

Also, "for PTP feature for ..."? How about "enable PTP hardware
timestamping and clock manipulation support for ..."?

> +	  of switch.

switches

> +
>  config NET_DSA_MICROCHIP_KSZ8863_SMI
>  	tristate "KSZ series SMI connected switch driver"
>  	depends on NET_DSA_MICROCHIP_KSZ_COMMON
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> new file mode 100644
> index 000000000000..cad0c6087419
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Microchip LAN937X PTP Implementation
> + * Copyright (C) 2021-2022 Microchip Technology Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/ptp_classify.h>
> +#include <linux/ptp_clock_kernel.h>
> +
> +#include "ksz_common.h"
> +#include "ksz_ptp.h"
> +#include "ksz_ptp_reg.h"
> +
> +#define ptp_caps_to_data(d) \
> +		container_of((d), struct ksz_ptp_data, caps)
> +#define ptp_data_to_ksz_dev(d) \
> +		container_of((d), struct ksz_device, ptp_data)
> +
> +#define MAX_DRIFT_CORR 6250000

KSZ_MAX_DRIFT_CORR maybe? Also maybe a small comment about the
assumptions that were made when it was calculated?

> +
> +#define KSZ_PTP_INC_NS 40  /* HW clock is incremented every 40 ns (by 40) */
> +#define KSZ_PTP_SUBNS_BITS 32  /* Number of bits in sub-nanoseconds counter */
> +
> +/* The function is return back the capability of timestamping feature when
> + * requested through ethtool -T <interface> utility
> + */
> +int ksz_get_ts_info(struct dsa_switch *ds, int port, struct ethtool_ts_info *ts)
> +{
> +	struct ksz_device *dev	= ds->priv;
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +
> +	ts->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> +			      SOF_TIMESTAMPING_RX_HARDWARE |
> +			      SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	ts->tx_types = (1 << HWTSTAMP_TX_OFF);
> +
> +	ts->rx_filters = (1 << HWTSTAMP_FILTER_NONE);
> +
> +	ts->phc_index = ptp_clock_index(ptp_data->clock);

Ah, but I don't think the optionality of ptp_data->clock is dealt with
very well here. ptp_data->clock can be NULL, and ethtool -T can still be
run on the interface. That will dereference a NULL pointer in ptp_clock_index().

int ptp_clock_index(struct ptp_clock *ptp)
{
	return ptp->index;
}
EXPORT_SYMBOL(ptp_clock_index);

> +
> +	return 0;
> +}
> +
> +int ksz_ptp_clock_register(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> +	int ret;
> +
> +	mutex_init(&ptp_data->lock);
> +
> +	ptp_data->caps = ksz_ptp_caps;
> +
> +	/* Start hardware counter */
> +	ret = ksz_ptp_start_clock(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Register the PTP Clock */
> +	ptp_data->clock = ptp_clock_register(&ptp_data->caps, dev->dev);
> +	if (IS_ERR_OR_NULL(ptp_data->clock))
> +		return PTR_ERR(ptp_data->clock);
> +
> +	ret = ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_802_1AS, PTP_802_1AS);

A small comment as to what this does? I see in other places you're
generous with comments, like "Register the PTP clock" above the
ptp_clock_register() call.

> +	if (ret)
> +		goto error_unregister_clock;
> +
> +	return 0;
> +
> +error_unregister_clock:
> +	ptp_clock_unregister(ptp_data->clock);
> +	return ret;
> +}
> +
> +MODULE_AUTHOR("Christian Eggers <ceggers@arri.de>");
> +MODULE_AUTHOR("Arun Ramadoss <arun.ramadoss@microchip.com>");
> +MODULE_DESCRIPTION("PTP support for KSZ switch");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.h b/drivers/net/dsa/microchip/ksz_ptp.h
> new file mode 100644
> index 000000000000..ac53b0df2733
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz_ptp.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Microchip LAN937X PTP Implementation
> + * Copyright (C) 2020-2021 Microchip Technology Inc.
> + */
> +
> +#ifndef _NET_DSA_DRIVERS_KSZ_PTP_H
> +#define _NET_DSA_DRIVERS_KSZ_PTP_H
> +
> +#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
> +
> +#endif	/* End of CONFIG_NET_DSA_MICROCHIOP_KSZ_PTP */

MICROCHIP not MICROCHIOP

> +
> +#endif