Received-SPF: pass (google.com: domain of linux-kernel+bounces-156762-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <adf60fa5-052f-4135-acab-91a02a9aff61@linux.dev>
Date: Wed, 24 Apr 2024 11:57:10 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_EXTTS
 for lan8814
To: Horatiu Vultur <horatiu.vultur@microchip.com>, andrew@lunn.ch,
 hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
 edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
 richardcochran@gmail.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240423195732.3353522-1-horatiu.vultur@microchip.com>
Content-Language: en-US
From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
In-Reply-To: <20240423195732.3353522-1-horatiu.vultur@microchip.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 23/04/2024 20:57, Horatiu Vultur wrote:
> Extend the PTP programmable gpios to implement also PTP_PF_EXTTS
> function. The pins can be configured to capture both of rising
> and falling edge. Once the event is seen, then an interrupt is
> generated and the LTC is saved in the registers.
> On lan8814 only GPIO 3 can be configured for this.
> 
> This was tested using:
> ts2phc -m -l 7 -s generic -f ts2phc.cfg
> 
> Where the configuration was the following:
> ---
> [global]
> ts2phc.pin_index  3
> 
> [eth0]
> ---
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

I'm not sure what happened to (fac63186f116 net: phy: micrel: Add
support for PTP_PF_EXTTS for lan8841), looks like this patch is the
rework previous with the limit to GPIO 3 only. In this case comments
below are applicable.

> ---
>   drivers/net/phy/micrel.c | 182 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 181 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 0e310a5e2bff0..2d11f38cbc243 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -167,6 +167,9 @@
>   #define PTP_CMD_CTL_PTP_LTC_STEP_SEC_		BIT(5)
>   #define PTP_CMD_CTL_PTP_LTC_STEP_NSEC_		BIT(6)
>   
> +#define PTP_COMMON_INT_ENA			0x0204
> +#define PTP_COMMON_INT_ENA_GPIO_CAP_EN		BIT(2)
> +
>   #define PTP_CLOCK_SET_SEC_HI			0x0205
>   #define PTP_CLOCK_SET_SEC_MID			0x0206
>   #define PTP_CLOCK_SET_SEC_LO			0x0207
> @@ -179,6 +182,27 @@
>   #define PTP_CLOCK_READ_NS_HI			0x022C
>   #define PTP_CLOCK_READ_NS_LO			0x022D
>   
> +#define PTP_GPIO_SEL				0x0230
> +#define PTP_GPIO_SEL_GPIO_SEL(pin)		((pin) << 8)
> +#define PTP_GPIO_CAP_MAP_LO			0x0232
> +
> +#define PTP_GPIO_CAP_EN				0x0233
> +#define PTP_GPIO_CAP_EN_GPIO_RE_CAPTURE_ENABLE(gpio)	BIT(gpio)
> +#define PTP_GPIO_CAP_EN_GPIO_FE_CAPTURE_ENABLE(gpio)	(BIT(gpio) << 8)
> +
> +#define PTP_GPIO_RE_LTC_SEC_HI_CAP		0x0235
> +#define PTP_GPIO_RE_LTC_SEC_LO_CAP		0x0236
> +#define PTP_GPIO_RE_LTC_NS_HI_CAP		0x0237
> +#define PTP_GPIO_RE_LTC_NS_LO_CAP		0x0238
> +#define PTP_GPIO_FE_LTC_SEC_HI_CAP		0x0239
> +#define PTP_GPIO_FE_LTC_SEC_LO_CAP		0x023A
> +#define PTP_GPIO_FE_LTC_NS_HI_CAP		0x023B
> +#define PTP_GPIO_FE_LTC_NS_LO_CAP		0x023C
> +
> +#define PTP_GPIO_CAP_STS			0x023D
> +#define PTP_GPIO_CAP_STS_PTP_GPIO_RE_STS(gpio)	BIT(gpio)
> +#define PTP_GPIO_CAP_STS_PTP_GPIO_FE_STS(gpio)	(BIT(gpio) << 8)
> +
>   #define PTP_OPERATING_MODE			0x0241
>   #define PTP_OPERATING_MODE_STANDALONE_		BIT(0)
>   
> @@ -274,6 +298,7 @@
>   
>   #define LAN8814_PTP_GPIO_NUM			24
>   #define LAN8814_PTP_PEROUT_NUM			2
> +#define LAN8814_PTP_EXTTS_NUM			3
>   
>   #define LAN8814_BUFFER_TIME			2
>   
> @@ -3124,12 +3149,102 @@ static int lan8814_ptp_perout(struct ptp_clock_info *ptpci,
>   	return 0;
>   }
>   
> +static void lan8814_ptp_extts_on(struct phy_device *phydev, int pin, u32 flags)
> +{
> +	u16 tmp;
> +
> +	/* Set as gpio input */
> +	tmp = lanphy_read_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin));
> +	tmp &= ~LAN8814_GPIO_DIR_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin), tmp);
> +
> +	/* Map the pin to ltc pin 0 of the capture map registers */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_CAP_MAP_LO);
> +	tmp |= pin;
> +	lanphy_write_page_reg(phydev, 4, PTP_GPIO_CAP_MAP_LO, tmp);
> +
> +	/* Enable capture on the edges of the ltc pin */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_CAP_EN);
> +	if (flags & PTP_RISING_EDGE)
> +		tmp |= PTP_GPIO_CAP_EN_GPIO_RE_CAPTURE_ENABLE(0);
> +	if (flags & PTP_FALLING_EDGE)
> +		tmp |= PTP_GPIO_CAP_EN_GPIO_FE_CAPTURE_ENABLE(0);
> +	lanphy_write_page_reg(phydev, 4, PTP_GPIO_CAP_EN, tmp);
> +
> +	/* Enable interrupt top interrupt */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_COMMON_INT_ENA);
> +	tmp |= PTP_COMMON_INT_ENA_GPIO_CAP_EN;
> +	lanphy_write_page_reg(phydev, 4, PTP_COMMON_INT_ENA, tmp);
> +}
> +
> +static void lan8814_ptp_extts_off(struct phy_device *phydev, int pin)
> +{
> +	u16 tmp;
> +
> +	/* Set as gpio out */
> +	tmp = lanphy_read_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin));
> +	tmp |= LAN8814_GPIO_DIR_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_DIR_ADDR(pin), tmp);
> +
> +	/* Enable alternate, 0:for alternate function, 1:gpio */
> +	tmp = lanphy_read_page_reg(phydev, 4, LAN8814_GPIO_EN_ADDR(pin));
> +	tmp &= ~LAN8814_GPIO_EN_BIT(pin);
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN_ADDR(pin), tmp);
> +
> +	/* Clear the mapping of pin to registers 0 of the capture registers */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_CAP_MAP_LO);
> +	tmp &= ~GENMASK(3, 0);
> +	lanphy_write_page_reg(phydev, 4, PTP_GPIO_CAP_MAP_LO, tmp);
> +
> +	/* Disable capture on both of the edges */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_CAP_EN);
> +	tmp &= ~PTP_GPIO_CAP_EN_GPIO_RE_CAPTURE_ENABLE(pin);
> +	tmp &= ~PTP_GPIO_CAP_EN_GPIO_FE_CAPTURE_ENABLE(pin);
> +	lanphy_write_page_reg(phydev, 4, PTP_GPIO_CAP_EN, tmp);
> +
> +	/* Disable interrupt top interrupt */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_COMMON_INT_ENA);
> +	tmp &= ~PTP_COMMON_INT_ENA_GPIO_CAP_EN;
> +	lanphy_write_page_reg(phydev, 4, PTP_COMMON_INT_ENA, tmp);
> +}
> +
> +static int lan8814_ptp_extts(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;
> +	int pin;
> +
> +	if (rq->extts.flags & ~(PTP_ENABLE_FEATURE |
> +				PTP_EXTTS_EDGES |
> +				PTP_STRICT_FLAGS))
> +		return -EOPNOTSUPP;
> +
> +	pin = ptp_find_pin(shared->ptp_clock, PTP_PF_EXTTS,
> +			   rq->extts.index);
> +	if (pin == -1 || pin != LAN8814_PTP_EXTTS_NUM)
> +		return -EINVAL;

I'm not sure how will enable request pass this check?
In lan8814_ptp_probe_once pins are initialized with PTP_PF_NONE,
and ptp_find_pin will always return -1, which will end up with
-EINVAL here and never hit lan8814_ptp_extts_on/lan8814_ptp_extts_off

> +
> +	mutex_lock(&shared->shared_lock);
> +	if (on)
> +		lan8814_ptp_extts_on(phydev, pin, rq->extts.flags);
> +	else
> +		lan8814_ptp_extts_off(phydev, pin);
> +
> +	mutex_unlock(&shared->shared_lock);
> +
> +	return 0;
> +}
> +
>   static int lan8814_ptpci_enable(struct ptp_clock_info *ptpci,
>   				struct ptp_clock_request *rq, int on)
>   {
>   	switch (rq->type) {
>   	case PTP_CLK_REQ_PEROUT:
>   		return lan8814_ptp_perout(ptpci, rq, on);
> +	case PTP_CLK_REQ_EXTTS:
> +		return lan8814_ptp_extts(ptpci, rq, on);
>   	default:
>   		return -EINVAL;
>   	}
> @@ -3148,6 +3263,10 @@ static int lan8814_ptpci_verify(struct ptp_clock_info *ptp, unsigned int pin,
>   		if (pin >= LAN8814_PTP_PEROUT_NUM || pin != chan)
>   			return -1;
>   		break;
> +	case PTP_PF_EXTTS:
> +		if (pin != LAN8814_PTP_EXTTS_NUM)

Here the check states that exactly GPIO 3 can have EXTTS function, but
later in the config...

> +			return -1;
> +		break;
>   	default:
>   		return -1;
>   	}
> @@ -3320,6 +3439,64 @@ static void lan8814_handle_ptp_interrupt(struct phy_device *phydev, u16 status)
>   	}
>   }
>   
> +static int lan8814_gpio_process_cap(struct lan8814_shared_priv *shared)
> +{
> +	struct phy_device *phydev = shared->phydev;
> +	struct ptp_clock_event ptp_event = {0};
> +	unsigned long nsec;
> +	s64 sec;
> +	u16 tmp;
> +
> +	/* This is 0 because whatever was the input pin it was mapped it to
> +	 * ltc gpio pin 0
> +	 */
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_SEL);
> +	tmp |= PTP_GPIO_SEL_GPIO_SEL(0);
> +	lanphy_write_page_reg(phydev, 4, PTP_GPIO_SEL, tmp);
> +
> +	tmp = lanphy_read_page_reg(phydev, 4, PTP_GPIO_CAP_STS);
> +	if (!(tmp & PTP_GPIO_CAP_STS_PTP_GPIO_RE_STS(0)) &&
> +	    !(tmp & PTP_GPIO_CAP_STS_PTP_GPIO_FE_STS(0)))
> +		return -1;
> +
> +	if (tmp & BIT(0)) {
> +		sec = lanphy_read_page_reg(phydev, 4, PTP_GPIO_RE_LTC_SEC_HI_CAP);
> +		sec <<= 16;
> +		sec |= lanphy_read_page_reg(phydev, 4, PTP_GPIO_RE_LTC_SEC_LO_CAP);
> +
> +		nsec = lanphy_read_page_reg(phydev, 4, PTP_GPIO_RE_LTC_NS_HI_CAP) & 0x3fff;
> +		nsec <<= 16;
> +		nsec |= lanphy_read_page_reg(phydev, 4, PTP_GPIO_RE_LTC_NS_LO_CAP);
> +	} else {
> +		sec = lanphy_read_page_reg(phydev, 4, PTP_GPIO_FE_LTC_SEC_HI_CAP);
> +		sec <<= 16;
> +		sec |= lanphy_read_page_reg(phydev, 4, PTP_GPIO_FE_LTC_SEC_LO_CAP);
> +
> +		nsec = lanphy_read_page_reg(phydev, 4, PTP_GPIO_FE_LTC_NS_HI_CAP) & 0x3fff;
> +		nsec <<= 16;
> +		nsec |= lanphy_read_page_reg(phydev, 4, PTP_GPIO_RE_LTC_NS_LO_CAP);
> +	}
> +
> +	ptp_event.index = 0;
> +	ptp_event.timestamp = ktime_set(sec, nsec);
> +	ptp_event.type = PTP_CLOCK_EXTTS;
> +	ptp_clock_event(shared->ptp_clock, &ptp_event);
> +
> +	return 0;
> +}
> +
> +static int lan8814_handle_gpio_interrupt(struct phy_device *phydev, u16 status)
> +{
> +	struct lan8814_shared_priv *shared = phydev->shared->priv;
> +	int ret;
> +
> +	mutex_lock(&shared->shared_lock);
> +	ret = lan8814_gpio_process_cap(shared);
> +	mutex_unlock(&shared->shared_lock);
> +
> +	return ret;
> +}
> +
>   static int lan8804_config_init(struct phy_device *phydev)
>   {
>   	int val;
> @@ -3424,6 +3601,9 @@ static irqreturn_t lan8814_handle_interrupt(struct phy_device *phydev)
>   		ret = IRQ_HANDLED;
>   	}
>   
> +	if (!lan8814_handle_gpio_interrupt(phydev, irq_status))
> +		ret = IRQ_HANDLED;
> +
>   	return ret;
>   }
>   
> @@ -3541,7 +3721,7 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev)
>   	snprintf(shared->ptp_clock_info.name, 30, "%s", phydev->drv->name);
>   	shared->ptp_clock_info.max_adj = 31249999;
>   	shared->ptp_clock_info.n_alarm = 0;
> -	shared->ptp_clock_info.n_ext_ts = 0;
> +	shared->ptp_clock_info.n_ext_ts = LAN8814_PTP_EXTTS_NUM;

Here ptp_clock is configured to have 3 pins supporting EXTTS.
Looks like it should be n_ext_ts = 1;

>   	shared->ptp_clock_info.n_pins = LAN8814_PTP_GPIO_NUM;
>   	shared->ptp_clock_info.pps = 0;
>   	shared->ptp_clock_info.pin_config = shared->pin_config;