Prepare for future hardware timestamp selection by adding source and
corresponding pointers to ptp_clock structure. Additionally, introduce
helpers for registering specific phydev or netdev PTP clocks, retrieving
PTP clock information such as hwtstamp source or phydev/netdev pointers,
and obtaining the ptp_clock structure from the phc index.
Signed-off-by: Kory Maincent <[email protected]>
---
Change in v8:
- New patch.
Change in v10:
- Add get and put function to avoid unregistering a ptp clock object used.
- Fix kdoc issues.
---
drivers/ptp/ptp_clock.c | 114 +++++++++++++++++++++++++++++++++++++++
drivers/ptp/ptp_private.h | 5 ++
include/linux/ptp_clock_kernel.h | 104 +++++++++++++++++++++++++++++++++++
3 files changed, 223 insertions(+)
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index c56cd0f63909..f962f460ec9d 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -512,6 +512,120 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
}
EXPORT_SYMBOL(ptp_cancel_worker_sync);
+struct ptp_clock *netdev_ptp_clock_register(struct ptp_clock_info *info,
+ struct net_device *dev)
+{
+ struct ptp_clock *ptp;
+
+ ptp = ptp_clock_register(info, &dev->dev);
+ if (IS_ERR(ptp))
+ return ptp;
+
+ ptp->phc_source = HWTSTAMP_SOURCE_NETDEV;
+ ptp->netdev = dev;
+
+ return ptp;
+}
+EXPORT_SYMBOL(netdev_ptp_clock_register);
+
+struct ptp_clock *phydev_ptp_clock_register(struct ptp_clock_info *info,
+ struct phy_device *phydev)
+{
+ struct ptp_clock *ptp;
+
+ ptp = ptp_clock_register(info, &phydev->mdio.dev);
+ if (IS_ERR(ptp))
+ return ptp;
+
+ ptp->phc_source = HWTSTAMP_SOURCE_PHYLIB;
+ ptp->phydev = phydev;
+
+ return ptp;
+}
+EXPORT_SYMBOL(phydev_ptp_clock_register);
+
+bool ptp_clock_from_phylib(struct ptp_clock *ptp)
+{
+ return ptp->phc_source == HWTSTAMP_SOURCE_PHYLIB;
+}
+EXPORT_SYMBOL(ptp_clock_from_phylib);
+
+bool ptp_clock_from_netdev(struct ptp_clock *ptp)
+{
+ return ptp->phc_source == HWTSTAMP_SOURCE_NETDEV;
+}
+EXPORT_SYMBOL(ptp_clock_from_netdev);
+
+struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
+{
+ if (ptp->phc_source != HWTSTAMP_SOURCE_NETDEV)
+ return NULL;
+
+ return ptp->netdev;
+}
+EXPORT_SYMBOL(ptp_clock_netdev);
+
+struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
+{
+ if (ptp->phc_source != HWTSTAMP_SOURCE_PHYLIB)
+ return NULL;
+
+ return ptp->phydev;
+}
+EXPORT_SYMBOL(ptp_clock_phydev);
+
+int ptp_clock_get(struct device *dev, struct ptp_clock *ptp)
+{
+ struct device_link *link;
+
+ if (!ptp)
+ return 0;
+
+ if (!try_module_get(ptp->info->owner))
+ return -EPROBE_DEFER;
+
+ get_device(&ptp->dev);
+
+ link = device_link_add(dev, &ptp->dev, DL_FLAG_STATELESS);
+ if (!link)
+ dev_warn(dev, "failed to create device link to %s\n",
+ dev_name(&ptp->dev));
+
+ return 0;
+}
+EXPORT_SYMBOL(ptp_clock_get);
+
+struct ptp_clock *ptp_clock_get_by_index(struct device *dev, int index)
+{
+ struct ptp_clock *ptp;
+ int ret;
+
+ if (index < 0)
+ return NULL;
+
+ ptp = xa_load(&ptp_clocks_map, (unsigned long)index);
+ if (IS_ERR_OR_NULL(ptp))
+ return ptp;
+
+ ret = ptp_clock_get(dev, ptp);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ptp;
+}
+EXPORT_SYMBOL(ptp_clock_get_by_index);
+
+void ptp_clock_put(struct device *dev, struct ptp_clock *ptp)
+{
+ if (!ptp)
+ return;
+
+ device_link_remove(dev, &ptp->dev);
+ put_device(&ptp->dev);
+ module_put(ptp->info->owner);
+}
+EXPORT_SYMBOL(ptp_clock_put);
+
/* module operations */
static void __exit ptp_exit(void)
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 18934e28469e..64d4bfafabfc 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -41,6 +41,11 @@ struct ptp_clock {
struct ptp_clock_info *info;
dev_t devid;
int index; /* index into clocks.map */
+ enum hwtstamp_source phc_source;
+ union { /* Pointer of the phc_source device */
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ };
struct pps_device *pps_source;
long dialed_frequency; /* remembers the frequency adjustment */
struct list_head tsevqs; /* timestamp fifo list */
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6e4b8206c7d0..367453dd3ada 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -9,7 +9,9 @@
#define _PTP_CLOCK_KERNEL_H_
#include <linux/device.h>
+#include <linux/netdevice.h>
#include <linux/pps_kernel.h>
+#include <linux/phy.h>
#include <linux/ptp_clock.h>
#include <linux/timecounter.h>
#include <linux/skbuff.h>
@@ -340,6 +342,90 @@ extern void ptp_clock_event(struct ptp_clock *ptp,
extern int ptp_clock_index(struct ptp_clock *ptp);
+/**
+ * netdev_ptp_clock_register() - register a PTP hardware clock driver for
+ * a net device
+ *
+ * @info: Structure describing the new clock.
+ * @dev: Pointer of the net device
+ */
+
+extern struct ptp_clock *
+netdev_ptp_clock_register(struct ptp_clock_info *info,
+ struct net_device *dev);
+
+/**
+ * phydev_ptp_clock_register() - register a PTP hardware clock driver for
+ * a phy device
+ *
+ * @info: Structure describing the new clock.
+ * @phydev: Pointer of the phy device
+ */
+
+extern struct ptp_clock *
+phydev_ptp_clock_register(struct ptp_clock_info *info,
+ struct phy_device *phydev);
+
+/**
+ * ptp_clock_from_phylib() - return true if the PTP clock comes from phylib
+ *
+ * @ptp: The clock obtained from net/phy_ptp_clock_register().
+ */
+
+bool ptp_clock_from_phylib(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_from_netdev() - return true if the PTP clock comes from netdev
+ *
+ * @ptp: The clock obtained from net/phy_ptp_clock_register().
+ */
+
+bool ptp_clock_from_netdev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_netdev() - obtain the net_device of PTP clock
+ *
+ * @ptp: The clock obtained from netdev_ptp_clock_register().
+ */
+
+struct net_device *ptp_clock_netdev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_phydev() - obtain the phy_device of a PTP clock
+ *
+ * @ptp: The clock obtained from phydev_ptp_clock_register().
+ */
+
+struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_get() - increment refcount of the PTP clock
+ *
+ * @dev: The device which get the PTP clock.
+ * @ptp: Pointer of a PTP clock.
+ */
+
+int ptp_clock_get(struct device *dev, struct ptp_clock *ptp);
+
+/**
+ * ptp_clock_get_by_index() - obtain the PTP clock reference from a given
+ * PHC index
+ *
+ * @dev: The device which get the PTP clock.
+ * @index: The device index of a PTP clock.
+ */
+
+struct ptp_clock *ptp_clock_get_by_index(struct device *dev, int index);
+
+/**
+ * ptp_clock_put() - decrement refcount of the PTP clock
+ *
+ * @dev: The device which get the PTP clock.
+ * @ptp: Pointer of a PTP clock.
+ */
+
+void ptp_clock_put(struct device *dev, struct ptp_clock *ptp);
+
/**
* ptp_find_pin() - obtain the pin index of a given auxiliary function
*
@@ -405,6 +491,24 @@ static inline void ptp_clock_event(struct ptp_clock *ptp,
{ }
static inline int ptp_clock_index(struct ptp_clock *ptp)
{ return -1; }
+static inline struct ptp_clock *
+netdev_ptp_clock_register(struct ptp_clock_info *info,
+ struct net_device *dev)
+{ return NULL; }
+static inline struct ptp_clock *
+phydev_ptp_clock_register(struct ptp_clock_info *info,
+ struct phy_device *phydev)
+{ return NULL; }
+static inline bool ptp_clock_from_phylib(struct ptp_clock *ptp)
+{ return false; }
+static inline bool ptp_clock_from_netdev(struct ptp_clock *ptp)
+{ return false; }
+static inline struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
+{ return NULL; }
+static inline struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp);
+{ return NULL; }
+static inline struct ptp_clock *ptp_clock_get_by_index(int index);
+{ return NULL; }
static inline int ptp_find_pin(struct ptp_clock *ptp,
enum ptp_pin_function func, unsigned int chan)
{ return -1; }
--
2.34.1
Kory Maincent wrote:
> Prepare for future hardware timestamp selection by adding source and
> corresponding pointers to ptp_clock structure. Additionally, introduce
> helpers for registering specific phydev or netdev PTP clocks, retrieving
> PTP clock information such as hwtstamp source or phydev/netdev pointers,
> and obtaining the ptp_clock structure from the phc index.
>
> Signed-off-by: Kory Maincent <[email protected]>
> ---
>
> Change in v8:
> - New patch.
>
> Change in v10:
> - Add get and put function to avoid unregistering a ptp clock object used.
> - Fix kdoc issues.
> ---
> drivers/ptp/ptp_clock.c | 114 +++++++++++++++++++++++++++++++++++++++
> drivers/ptp/ptp_private.h | 5 ++
> include/linux/ptp_clock_kernel.h | 104 +++++++++++++++++++++++++++++++++++
> 3 files changed, 223 insertions(+)
>
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index c56cd0f63909..f962f460ec9d 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -512,6 +512,120 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
> }
> EXPORT_SYMBOL(ptp_cancel_worker_sync);
>
> +struct ptp_clock *netdev_ptp_clock_register(struct ptp_clock_info *info,
> + struct net_device *dev)
> +{
> + struct ptp_clock *ptp;
> +
> + ptp = ptp_clock_register(info, &dev->dev);
> + if (IS_ERR(ptp))
> + return ptp;
> +
> + ptp->phc_source = HWTSTAMP_SOURCE_NETDEV;
> + ptp->netdev = dev;
> +
> + return ptp;
> +}
> +EXPORT_SYMBOL(netdev_ptp_clock_register);
> +
> +struct ptp_clock *phydev_ptp_clock_register(struct ptp_clock_info *info,
> + struct phy_device *phydev)
> +{
> + struct ptp_clock *ptp;
> +
> + ptp = ptp_clock_register(info, &phydev->mdio.dev);
> + if (IS_ERR(ptp))
> + return ptp;
> +
> + ptp->phc_source = HWTSTAMP_SOURCE_PHYLIB;
> + ptp->phydev = phydev;
> +
> + return ptp;
> +}
> +EXPORT_SYMBOL(phydev_ptp_clock_register);
> +
> +bool ptp_clock_from_phylib(struct ptp_clock *ptp)
> +{
> + return ptp->phc_source == HWTSTAMP_SOURCE_PHYLIB;
> +}
> +EXPORT_SYMBOL(ptp_clock_from_phylib);
> +
> +bool ptp_clock_from_netdev(struct ptp_clock *ptp)
> +{
> + return ptp->phc_source == HWTSTAMP_SOURCE_NETDEV;
> +}
> +EXPORT_SYMBOL(ptp_clock_from_netdev);
> +
> +struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
> +{
> + if (ptp->phc_source != HWTSTAMP_SOURCE_NETDEV)
> + return NULL;
> +
> + return ptp->netdev;
> +}
> +EXPORT_SYMBOL(ptp_clock_netdev);
> +
> +struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
> +{
> + if (ptp->phc_source != HWTSTAMP_SOURCE_PHYLIB)
> + return NULL;
> +
> + return ptp->phydev;
> +}
> +EXPORT_SYMBOL(ptp_clock_phydev);
IMHO these four helpers just add a layer of indirection without much
benefit.
Do we ever expect more than two sources? Else, the phc_source could be
embedded as the least significant bit of the pointer in the union. In
that case we would need helpers to return the pointer without that LSB.
But space in struct ptp_clock is probably not so valuable that we need
to play such games.
> +/**
> + * netdev_ptp_clock_register() - register a PTP hardware clock driver for
> + * a net device
> + *
> + * @info: Structure describing the new clock.
> + * @dev: Pointer of the net device
> + */
> +
> +extern struct ptp_clock *
> +netdev_ptp_clock_register(struct ptp_clock_info *info,
> + struct net_device *dev);
No need for explicit extern?
Willem de Bruijn wrote:
> Kory Maincent wrote:
> > Prepare for future hardware timestamp selection by adding source and
> > corresponding pointers to ptp_clock structure. Additionally, introduce
> > helpers for registering specific phydev or netdev PTP clocks, retrieving
> > PTP clock information such as hwtstamp source or phydev/netdev pointers,
> > and obtaining the ptp_clock structure from the phc index.
> >
> > Signed-off-by: Kory Maincent <[email protected]>
> > ---
> >
> > Change in v8:
> > - New patch.
> >
> > Change in v10:
> > - Add get and put function to avoid unregistering a ptp clock object used.
> > - Fix kdoc issues.
> > ---
> > drivers/ptp/ptp_clock.c | 114 +++++++++++++++++++++++++++++++++++++++
> > drivers/ptp/ptp_private.h | 5 ++
> > include/linux/ptp_clock_kernel.h | 104 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 223 insertions(+)
> >
> > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> > index c56cd0f63909..f962f460ec9d 100644
> > --- a/drivers/ptp/ptp_clock.c
> > +++ b/drivers/ptp/ptp_clock.c
> > @@ -512,6 +512,120 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
> > }
> > EXPORT_SYMBOL(ptp_cancel_worker_sync);
> >
> > +struct ptp_clock *netdev_ptp_clock_register(struct ptp_clock_info *info,
> > + struct net_device *dev)
> > +{
> > + struct ptp_clock *ptp;
> > +
> > + ptp = ptp_clock_register(info, &dev->dev);
> > + if (IS_ERR(ptp))
> > + return ptp;
> > +
> > + ptp->phc_source = HWTSTAMP_SOURCE_NETDEV;
> > + ptp->netdev = dev;
> > +
> > + return ptp;
> > +}
> > +EXPORT_SYMBOL(netdev_ptp_clock_register);
> > +
> > +struct ptp_clock *phydev_ptp_clock_register(struct ptp_clock_info *info,
> > + struct phy_device *phydev)
> > +{
> > + struct ptp_clock *ptp;
> > +
> > + ptp = ptp_clock_register(info, &phydev->mdio.dev);
> > + if (IS_ERR(ptp))
> > + return ptp;
> > +
> > + ptp->phc_source = HWTSTAMP_SOURCE_PHYLIB;
> > + ptp->phydev = phydev;
> > +
> > + return ptp;
> > +}
> > +EXPORT_SYMBOL(phydev_ptp_clock_register);
> > +
> > +bool ptp_clock_from_phylib(struct ptp_clock *ptp)
> > +{
> > + return ptp->phc_source == HWTSTAMP_SOURCE_PHYLIB;
> > +}
> > +EXPORT_SYMBOL(ptp_clock_from_phylib);
> > +
> > +bool ptp_clock_from_netdev(struct ptp_clock *ptp)
> > +{
> > + return ptp->phc_source == HWTSTAMP_SOURCE_NETDEV;
> > +}
> > +EXPORT_SYMBOL(ptp_clock_from_netdev);
> > +
> > +struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
> > +{
> > + if (ptp->phc_source != HWTSTAMP_SOURCE_NETDEV)
> > + return NULL;
> > +
> > + return ptp->netdev;
> > +}
> > +EXPORT_SYMBOL(ptp_clock_netdev);
> > +
> > +struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
> > +{
> > + if (ptp->phc_source != HWTSTAMP_SOURCE_PHYLIB)
> > + return NULL;
> > +
> > + return ptp->phydev;
> > +}
> > +EXPORT_SYMBOL(ptp_clock_phydev);
>
> IMHO these four helpers just add a layer of indirection without much
> benefit.
Actually, ignore this. This is not important enough to revise when
we're alredy at v10. That also goes for the other points.
> Do we ever expect more than two sources? Else, the phc_source could be
> embedded as the least significant bit of the pointer in the union. In
> that case we would need helpers to return the pointer without that LSB.
> But space in struct ptp_clock is probably not so valuable that we need
> to play such games.
>
> > +/**
> > + * netdev_ptp_clock_register() - register a PTP hardware clock driver for
> > + * a net device
> > + *
> > + * @info: Structure describing the new clock.
> > + * @dev: Pointer of the net device
> > + */
> > +
> > +extern struct ptp_clock *
> > +netdev_ptp_clock_register(struct ptp_clock_info *info,
> > + struct net_device *dev);
>
> No need for explicit extern?
On Wed, 10 Apr 2024 23:00:53 -0400
Willem de Bruijn <[email protected]> wrote:
> > +/**
> > + * netdev_ptp_clock_register() - register a PTP hardware clock driver for
> > + * a net device
> > + *
> > + * @info: Structure describing the new clock.
> > + * @dev: Pointer of the net device
> > + */
> > +
> > +extern struct ptp_clock *
> > +netdev_ptp_clock_register(struct ptp_clock_info *info,
> > + struct net_device *dev);
>
> No need for explicit extern?
Indeed I don't think it is needed.
I am wondering why few other functions uses it (ptp_clock_register,
ptp_clock_event, ptp_clock_index). Do you know?
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Kory Maincent wrote:
> On Wed, 10 Apr 2024 23:00:53 -0400
> Willem de Bruijn <[email protected]> wrote:
>
> > > +/**
> > > + * netdev_ptp_clock_register() - register a PTP hardware clock driver for
> > > + * a net device
> > > + *
> > > + * @info: Structure describing the new clock.
> > > + * @dev: Pointer of the net device
> > > + */
> > > +
> > > +extern struct ptp_clock *
> > > +netdev_ptp_clock_register(struct ptp_clock_info *info,
> > > + struct net_device *dev);
> >
> > No need for explicit extern?
>
> Indeed I don't think it is needed.
> I am wondering why few other functions uses it (ptp_clock_register,
> ptp_clock_event, ptp_clock_index). Do you know?
Perhaps it predates the coding style rule
Do not use the ``extern`` keyword with function declarations as this makes
lines longer and isn't strictly necessary.