On Friday, 13 November 2020, 17:56:34 CET, Christian Eggers wrote:
> On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:
> > On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> > > 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@xxxxxxx>
> > > ---
> >
> > 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.
> >
> > 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.
>
> > You could even move
> > the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
> > can see.
regards
Christian
