On Friday, 13 November 2020, 17:56:34 CET, Christian Eggers wrote:

On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:

quoted

On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:

quoted

Parts of ksz_common.h (struct ksz_device) will be required in
net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
file.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---

I had to skip ahead to see what you're going to use struct ksz_port and

struct ksz_device for. It looks like you need:
	struct ksz_port::tstamp_rx_latency_ns
	struct ksz_device::ptp_clock_lock
	struct ksz_device::ptp_clock_time

Not more.

I have tried to put these members into separate structs:

include/linux/dsa/ksz_common.h:
struct ksz_port_ptp_shared {
	u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
};

struct ksz_device_ptp_shared {
	spinlock_t ptp_clock_lock; /* for ptp_clock_time */
	/* approximated current time, read once per second from hardware */
	struct timespec64 ptp_clock_time;
};

drivers/net/dsa/microchip/ksz_common.h:
...
#include <linux/dsa/ksz_common.h>
...
struct ksz_port {
...
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
	struct ksz_port_ptp_shared ptp_shared;	/* shared with tag_ksz.c */
	u16 tstamp_tx_latency_ns;	/* tx delay from tstamp unit to wire */
	struct hwtstamp_config tstamp_config;
	struct sk_buff *tstamp_tx_xdelay_skb;
	unsigned long tstamp_state;
#endif
};
...
struct ksz_device {
...
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
	struct ptp_clock *ptp_clock;
	struct ptp_clock_info ptp_caps;
	struct mutex ptp_mutex;
	struct ksz_device_ptp_shared ptp_shared;   /* shared with tag_ksz.c */
#endif
};

The problem with such technique is, that I still need to dereference
struct ksz_device in tag_ksz.c:

static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
				  struct net_device *dev, unsigned int port)
{
...
	struct dsa_switch *ds = dev->dsa_ptr->ds;
	struct ksz_device *ksz = ds->priv;
	struct ksz_port *prt = &ksz->ports[port];
...
}

As struct dsa_switch::priv is already occupied by the pointer to
struct ksz_device, I see no way accessing the ptp specific device/port
information in tag_ksz.c.

quoted

Why don't you go the other way around, i.e. exporting some functions
from your driver, and calling them from the tagger?

Good question... But as for as I can see, there are a single tagger and
multiple device drivers (currently KSZ8795 and KSZ9477).

Moving the KSZ9477 specific stuff, which is required by the tagger, into the
KSZ9477 device driver, would make the tagger dependent on the driver(s).
Currently, no tagger seems to have this direction of dependency (at least I
cannot find this in net/dsa/Kconfig).

If I shall change this anyway, I would use #ifdefs within the tag_ksz driver
in order to avoid unnecessary dependencies to the KSZ9477 driver for the
case only KSZ8795 is selected.

quoted

You could even move
the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
can see.

regards
Christian