Re: [PATCH net-next v3 5/9] net: phy: micrel: add HW timestamp configuration reporting

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 19/11/2025 14:44, Kory Maincent wrote:
> On Wed, 19 Nov 2025 12:47:21 +0000
> Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
> 
>> The driver stores HW timestamping configuration and can technically
>> report it. Add callback to do it.
>>
>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> ---
>>   drivers/net/phy/micrel.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 05de68b9f719..b149d539aec3 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -3147,6 +3147,18 @@ static void lan8814_flush_fifo(struct phy_device
>> *phydev, bool egress) lanphy_read_page_reg(phydev, LAN8814_PAGE_PORT_REGS,
>> PTP_TSU_INT_STS); }
>>   
>> +static int lan8814_hwtstamp_get(struct mii_timestamper *mii_ts,
>> +				struct kernel_hwtstamp_config *config)
>> +{
>> +	struct kszphy_ptp_priv *ptp_priv =
>> +			  container_of(mii_ts, struct kszphy_ptp_priv,
>> mii_ts); +
>> +	config->tx_type = ptp_priv->hwts_tx_type;
>> +	config->rx_filter = ptp_priv->rx_filter;
>> +
>> +	return 0;
>> +}
>> +
>>   static int lan8814_hwtstamp_set(struct mii_timestamper *mii_ts,
>>   				struct kernel_hwtstamp_config *config,
>>   				struct netlink_ext_ack *extack)
>> @@ -4390,6 +4402,7 @@ static void lan8814_ptp_init(struct phy_device *phydev)
>>   	ptp_priv->mii_ts.rxtstamp = lan8814_rxtstamp;
>>   	ptp_priv->mii_ts.txtstamp = lan8814_txtstamp;
>>   	ptp_priv->mii_ts.hwtstamp_set = lan8814_hwtstamp_set;
>> +	ptp_priv->mii_ts.hwtstamp_get = lan8814_hwtstamp_get;
>>   	ptp_priv->mii_ts.ts_info  = lan8814_ts_info;
>>   
>>   	phydev->mii_ts = &ptp_priv->mii_ts;
>> @@ -5042,6 +5055,19 @@ static void lan8841_ptp_enable_processing(struct
>> kszphy_ptp_priv *ptp_priv, #define LAN8841_PTP_TX_TIMESTAMP_EN
>> 443 #define LAN8841_PTP_TX_MOD			445
>>   
>> +static int lan8841_hwtstamp_get(struct mii_timestamper *mii_ts,
>> +				struct kernel_hwtstamp_config *config)
>> +{
>> +	struct kszphy_ptp_priv *ptp_priv;
>> +
>> +	ptp_priv = container_of(mii_ts, struct kszphy_ptp_priv, mii_ts);
>> +
>> +	config->tx_type = ptp_priv->hwts_tx_type;
>> +	config->rx_filter = ptp_priv->rx_filter;
> 
> Something related to this patch, it seems there is an issue in the set
> callback:
> https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/net/phy/micrel.c#L3056
> 
> The priv->rx_filter and priv->hwts_tx_type are set before the switch condition.
> The hwtstamp_get can then report something that is not supported.
> Also HWTSTAMP_TX_ONESTEP_P2P is not managed by the driver but not returning
> -ERANGE either so if we set this config the hwtstamp_get will report something
> wrong as not supported.
> I think you will need to add a new patch here to fix the hwtstamp_set ops.

I agree, that there is a problem in the flow, but such change is out of
scope of this patch set. I'm going to provide some logic improvements on
per-driver basis as follow up work.

> Maybe we should update net_hwtstamp_validate to check on the capabilities
> reported by ts_info but that is a bigger change.

In this case we have to introduce validation callback and implement it
in drivers. Some drivers do downgrade filter values if provided value is
not in the list of what was provided in ethtool::ts_info. And we have to
keep this logic as otherwise it may be considered as API breakage.

> 
>> +
>> +	return 0;
>> +}
>> +
>>   static int lan8841_hwtstamp_set(struct mii_timestamper *mii_ts,
>>   				struct kernel_hwtstamp_config *config,
>>   				struct netlink_ext_ack *extack)
>> @@ -5925,6 +5951,7 @@ static int lan8841_probe(struct phy_device *phydev)
>>   	ptp_priv->mii_ts.rxtstamp = lan8841_rxtstamp;
>>   	ptp_priv->mii_ts.txtstamp = lan8814_txtstamp;
>>   	ptp_priv->mii_ts.hwtstamp_set = lan8841_hwtstamp_set;
>> +	ptp_priv->mii_ts.hwtstamp_get = lan8841_hwtstamp_get;
>>   	ptp_priv->mii_ts.ts_info = lan8841_ts_info;
>>   
>>   	phydev->mii_ts = &ptp_priv->mii_ts;
> 
> 
>