RE: [PATCH net-next 2/2] net: phy: micrel: lan8814: Add support for PTP_PF_PEROUT

[Divya.Koppera]

Hi Horatiu,


> Subject: [PATCH net-next 2/2] net: phy: micrel: lan8814: Add support for
> PTP_PF_PEROUT
> 
> Lan8814 has 24 GPIOs but only 2 GPIOs (GPIO 0 and GPIO 1) can be
> configured to generate period signals. And there are 2 events (EVENT_A and
> EVENT_B) but these events are hardcoded to the GPIO 0 and GPIO 1.
> These events are used to generate period signals. It is possible to configure the
> length, the start time and the period of the signal by configuring the event.
> 
> These events are generated by comparing the target time with the PHC time.
> In case the PHC time is changed to a value bigger than the target time + reload
> time, then it would generate only 1 event and then it would stop because
> target time + reload time is smaller than PHC time.
> Therefore it is required to change also the target time every time when the PHC
> is changed. The same will apply also when the PHC time is changed to a smaller
> value.
> 
> This was tested using:
> testptp -i 1 -L 1,2
> testptp -i 1 -p 1000000000 -w 200000000
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> ---
>  drivers/net/phy/micrel.c | 353
> ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 351 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index
> 51ca1b2b5d99a..521c6f7ab420c 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -272,6 +272,66 @@
> +static void lan8814_ptp_perout_off(struct phy_device *phydev, int pin)
> +{
> +	u16 val;
> +
> +	/* Disable gpio alternate function,
> +	 * 1: select as gpio,
> +	 * 0: select alt func
> +	 */
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_EN_ADDR(pin));
> +	val |= LAN8814_GPIO_EN_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN_ADDR(pin),
> val);
> +
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_DIR_ADDR(pin));
> +	val &= ~LAN8814_GPIO_DIR_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin),
> val);
> +
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_BUF_ADDR(pin));
> +	val &= ~LAN8814_GPIO_BUF_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_BUF_ADDR(pin),
> val); }
> +
> +static void lan8814_ptp_perout_on(struct phy_device *phydev, int pin) {
> +	int val;
> +
> +	/* Set as gpio output */
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_DIR_ADDR(pin));
> +	val |= LAN8814_GPIO_DIR_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin),
> val);
> +
> +	/* Enable gpio 0:for alternate function, 1:gpio */
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_EN_ADDR(pin));
> +	val &= ~LAN8814_GPIO_EN_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN_ADDR(pin),
> val);
> +
> +	/* Set buffer type to push pull */
> +	val = lanphy_read_page_reg(phydev, 4,
> LAN8814_GPIO_BUF_ADDR(pin));
> +	val |= LAN8814_GPIO_BUF_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_BUF_ADDR(pin),
> val); }
> +
> +static int lan8814_ptp_perout(struct ptp_clock_info *ptpci,
> +			      struct ptp_clock_request *rq, int on) {
> +	struct lan8814_shared_priv *shared = container_of(ptpci, struct
> lan8814_shared_priv,
> +							  ptp_clock_info);
> +	struct phy_device *phydev = shared->phydev;
> +	struct timespec64 ts_on, ts_period;
> +	s64 on_nsec, period_nsec;
> +	int pulse_width;
> +	int pin, event;
> +
> +	/* Reject requests with unsupported flags */
> +	if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&shared->shared_lock);
> +	event = rq->perout.index;
> +	pin = ptp_find_pin(shared->ptp_clock, PTP_PF_PEROUT, event);
> +	if (pin < 0 || pin >= LAN8814_PTP_PEROUT_NUM) {
> +		mutex_unlock(&shared->shared_lock);
> +		return -EBUSY;
> +	}
> +
> +	if (!on) {
> +		lan8814_ptp_perout_off(phydev, pin);
> +		lan8814_ptp_disable_event(phydev, event);
> +		mutex_unlock(&shared->shared_lock);
> +		return 0;
> +	}
> +
> +	ts_on.tv_sec = rq->perout.on.sec;
> +	ts_on.tv_nsec = rq->perout.on.nsec;
> +	on_nsec = timespec64_to_ns(&ts_on);
> +
> +	ts_period.tv_sec = rq->perout.period.sec;
> +	ts_period.tv_nsec = rq->perout.period.nsec;
> +	period_nsec = timespec64_to_ns(&ts_period);
> +
> +	if (period_nsec < 200) {
> +		pr_warn_ratelimited("%s: perout period too small, minimum
> is 200 nsec\n",
> +				    phydev_name(phydev));
> +		return -EOPNOTSUPP;
> +	}

Unlock is Missing in above and below conditions.

> +
> +	if (on_nsec >= period_nsec) {
> +		pr_warn_ratelimited("%s: pulse width must be smaller than
> period\n",
> +				    phydev_name(phydev));
> +		return -EINVAL;
> +	}
> +
> +	switch (on_nsec) {
> +	case 200000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_200MS;
> +		break;
> +	case 100000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100MS;
> +		break;
> +	case 50000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50MS;
> +		break;
> +	case 10000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10MS;
> +		break;
> +	case 5000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5MS;
> +		break;
> +	case 1000000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1MS;
> +		break;
> +	case 500000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500US;
> +		break;
> +	case 100000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100US;
> +		break;
> +	case 50000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_50US;
> +		break;
> +	case 10000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_10US;
> +		break;
> +	case 5000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_5US;
> +		break;
> +	case 1000:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_1US;
> +		break;
> +	case 500:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_500NS;
> +		break;
> +	case 100:
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100NS;
> +		break;
> +	default:
> +		pr_warn_ratelimited("%s: Use default duty cycle of 100ns\n",
> +				    phydev_name(phydev));
> +		pulse_width =
> LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_100NS;
> +		break;
> +	}
> +
> +	/* Configure to pulse every period */
> +	lan8814_ptp_enable_event(phydev, event, pulse_width);
> +	lan8814_ptp_set_target(phydev, event, rq->perout.start.sec,
> +			       rq->perout.start.nsec);
> +	lan8814_ptp_set_reload(phydev, event, rq->perout.period.sec,
> +			       rq->perout.period.nsec);
> +	lan8814_ptp_perout_on(phydev, pin);
> +	mutex_unlock(&shared->shared_lock);
> +
> +	return 0;
> +}
> +
> 2.34.1

Except above comment, everything is good.

Reviewed-by: Divya Koppera <divya.koppera@xxxxxxxxxxxxx>