[Intel-wired-lan] [PATCH iwl-next v8 06/14] iavf: add initial framework for registering PTP clock
Mateusz Polchlopek
mateusz.polchlopek at intel.com
Thu Aug 8 11:04:29 UTC 2024
On 7/30/2024 3:40 PM, Alexander Lobakin wrote:
> From: Mateusz Polchlopek <mateusz.polchlopek at intel.com>
> Date: Tue, 30 Jul 2024 05:15:01 -0400
>
>> From: Jacob Keller <jacob.e.keller at intel.com>
>>
>> Add the iavf_ptp.c file and fill it in with a skeleton framework to
>> allow registering the PTP clock device.
>> Add implementation of helper functions to check if a PTP capability
>> is supported and handle change in PTP capabilities.
>> Enabling virtual clock would be possible, though it would probably
>> perform poorly due to the lack of direct time access.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek at intel.com>
>> Reviewed-by: Sai Krishna <saikrishnag at marvell.com>
>> Reviewed-by: Simon Horman <horms at kernel.org>
>> Co-developed-by: Ahmed Zaki <ahmed.zaki at intel.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki at intel.com>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek at intel.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek at intel.com>
>> ---
>> drivers/net/ethernet/intel/iavf/Makefile | 1 +
>> drivers/net/ethernet/intel/iavf/iavf_main.c | 5 +
>> drivers/net/ethernet/intel/iavf/iavf_ptp.c | 126 ++++++++++++++++++
>> drivers/net/ethernet/intel/iavf/iavf_ptp.h | 10 ++
>> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 2 +
>> 5 files changed, 144 insertions(+)
>> create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.c
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile
>> index 356ac9faa5bf..cff88cb49540 100644
>> --- a/drivers/net/ethernet/intel/iavf/Makefile
>> +++ b/drivers/net/ethernet/intel/iavf/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_IAVF) += iavf.o
>>
>> iavf-y := iavf_main.o iavf_ethtool.o iavf_virtchnl.o iavf_fdir.o \
>> iavf_adv_rss.o iavf_txrx.o iavf_common.o iavf_adminq.o
>> +iavf-$(CONFIG_PTP_1588_CLOCK) += iavf_ptp.o
>
> Newline before this one?
>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> index 11599d62d15a..3961c540c1c4 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>> @@ -2847,6 +2847,9 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
>> /* request initial VLAN offload settings */
>> iavf_set_vlan_offload_features(adapter, 0, netdev->features);
>>
>> + /* Setup initial PTP configuration */
>> + iavf_ptp_init(adapter);
>> +
>> iavf_schedule_finish_config(adapter);
>> return;
>>
>> @@ -5474,6 +5477,8 @@ static void iavf_remove(struct pci_dev *pdev)
>> msleep(50);
>> }
>>
>> + iavf_ptp_release(adapter);
>> +
>> iavf_misc_irq_disable(adapter);
>> /* Shut down all the garbage mashers on the detention level */
>> cancel_work_sync(&adapter->reset_task);
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
>> new file mode 100644
>> index 000000000000..1344298481d4
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c
>> @@ -0,0 +1,126 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright(c) 2024 Intel Corporation. */
>> +
>> +#include "iavf.h"
>> +
>> +/**
>> + * iavf_ptp_cap_supported - Check if a PTP capability is supported
>> + * @adapter: private adapter structure
>> + * @cap: the capability bitmask to check
>> + *
>> + * Return: true if every capability set in cap is also set in the enabled
>> + * capabilities reported by the PF, false otherwise.
>> + */
>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap)
>> +{
>> + if (!PTP_ALLOWED(adapter))
>> + return false;
>> +
>> + /* Only return true if every bit in cap is set in hw_caps.caps */
>> + return (adapter->ptp.hw_caps.caps & cap) == cap;
>
> Aren't these parenthesis redundant?
>
I think they are not. They wrap bit operation and also I checked it
with checkpatch script and it doesn't complain about reduntant
parenthesis.
>> +}
>> +
>> +/**
>> + * iavf_ptp_register_clock - Register a new PTP for userspace
>> + * @adapter: private adapter structure
>> + *
>> + * Allocate and register a new PTP clock device if necessary.
>> + *
>> + * Return: 0 if success, error otherwise
>> + */
>> +static int iavf_ptp_register_clock(struct iavf_adapter *adapter)
>> +{
>> + struct ptp_clock_info *ptp_info = &adapter->ptp.info;
>> + struct device *dev = &adapter->pdev->dev;
>> +
>> + memset(ptp_info, 0, sizeof(*ptp_info));
>> +
>> + snprintf(ptp_info->name, sizeof(ptp_info->name) - 1, "%s-%s-clk",
>
> sizeof(ptp_info->name) without `-1`, snprintf() takes care of it.
>
>> + dev_driver_string(dev),
>> + dev_name(dev));
>
> This can be placed in one line.
>
>> + ptp_info->owner = THIS_MODULE;
>> +
>> + adapter->ptp.clock = ptp_clock_register(ptp_info, dev);
>> + if (IS_ERR(adapter->ptp.clock))
>> + return PTR_ERR(adapter->ptp.clock);
>
> Shouldn't ptp.clock be nullified when this happens?
>
>> +
>> + dev_info(&adapter->pdev->dev, "PTP clock %s registered\n",
>> + adapter->ptp.info.name);
>
> 1. dev_dbg()?
> 2. pci_* for printing messages from PCI drivers.
> 3. Empty newline before return.
>
>> + return 0;
>> +}
>> +
>> +/**
>> + * iavf_ptp_init - Initialize PTP support if capability was negotiated
>> + * @adapter: private adapter structure
>> + *
>> + * Initialize PTP functionality, based on the capabilities that the PF has
>> + * enabled for this VF.
>> + */
>> +void iavf_ptp_init(struct iavf_adapter *adapter)
>> +{
>> + struct device *dev = &adapter->pdev->dev;
>> + int err;
>> +
>> + if (WARN_ON(adapter->ptp.initialized)) {
>> + dev_err(dev, "PTP functionality was already initialized!\n");
>
> Is this needed?
>
>> + return;
>> + }
>> +
>> + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) {
>> + dev_dbg(dev, "Device does not have PTP clock support\n");
>
> This is pci_warn(), not dbg I guess?
>
>> + return;
>> + }
>> +
>> + err = iavf_ptp_register_clock(adapter);
>> + if (err) {
>> + dev_warn(dev, "Failed to register PTP clock device (%d)\n",
>> + err);
>
> pci_warn(pdev, "failed to ... device, %pe\n",
> ERR_PTR(err));
>
>> + return;
>> + }
>> +
>> + adapter->ptp.initialized = true;
>> +}
>> +
>> +/**
>> + * iavf_ptp_release - Disable PTP support
>> + * @adapter: private adapter structure
>> + *
>> + * Release all PTP resources that were previously initialized.
>> + */
>> +void iavf_ptp_release(struct iavf_adapter *adapter)
>> +{
>> + adapter->ptp.initialized = false;
>> +
>> + if (!IS_ERR_OR_NULL(adapter->ptp.clock)) {
>> + dev_info(&adapter->pdev->dev, "removing PTP clock %s\n",
>> + adapter->ptp.info.name);
>
> _dbg().
>
>> + ptp_clock_unregister(adapter->ptp.clock);
>> + adapter->ptp.clock = NULL;
>> + }
>> +}
>> +
>> +/**
>> + * iavf_ptp_process_caps - Handle change in PTP capabilities
>> + * @adapter: private adapter structure
>> + *
>> + * Handle any state changes necessary due to change in PTP capabilities, such
>> + * as after a device reset or change in configuration from the PF.
>> + */
>> +void iavf_ptp_process_caps(struct iavf_adapter *adapter)
>> +{
>> + struct device *dev = &adapter->pdev->dev;
>> +
>> + dev_dbg(dev, "PTP capabilities changed at runtime\n");
>
> Is this needed at all?
>
>> +
>> + /* Check if the device gained or lost necessary access to support the
>> + * PTP hardware clock. If so, driver must respond appropriately by
>> + * creating or destroying the PTP clock device.
>> + */
>> + if (adapter->ptp.initialized &&
>> + !iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC))
>> + iavf_ptp_release(adapter);
>> + else if (!adapter->ptp.initialized &&
>> + iavf_ptp_cap_supported(adapter,
>> + VIRTCHNL_1588_PTP_CAP_READ_PHC))
>
> You can check for the cap support once before the whole block.
>
>> + iavf_ptp_init(adapter);
>> +}
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
>> index aee4e2da0b9a..4939c219bd18 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h
>> @@ -4,9 +4,19 @@
>> #ifndef _IAVF_PTP_H_
>> #define _IAVF_PTP_H_
>>
>> +#include <linux/ptp_clock_kernel.h>
>> +
>> /* fields used for PTP support */
>> struct iavf_ptp {
>> struct virtchnl_ptp_caps hw_caps;
>> + bool initialized;
>
> Holes.
>
>> + struct ptp_clock_info info;
>> + struct ptp_clock *clock;
>> };
>>
>> +void iavf_ptp_init(struct iavf_adapter *adapter);
>> +void iavf_ptp_release(struct iavf_adapter *adapter);
>> +void iavf_ptp_process_caps(struct iavf_adapter *adapter);
>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap);
>> +
>> #endif /* _IAVF_PTP_H_ */
>
> Thanks,
> Olek
More information about the Intel-wired-lan
mailing list