diff mbox series

[net-next,v2,03/11] net: dsa: microchip: split ksz_common.h

Message ID 20201112153537.22383-4-ceggers@arri.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: microchip: PTP support for KSZ956x | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 196 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Christian Eggers Nov. 12, 2020, 3:35 p.m. UTC
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>
---
 MAINTAINERS                            |  1 +
 drivers/net/dsa/microchip/ksz_common.h | 81 +---------------------
 include/linux/dsa/ksz_common.h         | 96 ++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 80 deletions(-)
 create mode 100644 include/linux/dsa/ksz_common.h

Comments

Vladimir Oltean Nov. 12, 2020, 11:02 p.m. UTC | #1
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@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.

Why don't you go the other way around, i.e. exporting some functions
from your driver, and calling them from the tagger? You could even move
the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
can see.
Vladimir Oltean Nov. 12, 2020, 11:04 p.m. UTC | #2
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@arri.de>
> ---
>  MAINTAINERS                            |  1 +
>  drivers/net/dsa/microchip/ksz_common.h | 81 +---------------------
>  include/linux/dsa/ksz_common.h         | 96 ++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+), 80 deletions(-)
>  create mode 100644 include/linux/dsa/ksz_common.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d173fcbf119..de7e2d80426a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11520,6 +11520,7 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>  F:	drivers/net/dsa/microchip/*
> +F:	include/linux/dsa/microchip/ksz_common.h

Ah, I almost forgot. This path is wrong, it has an extra "microchip".
Christian Eggers Nov. 13, 2020, 4:56 p.m. UTC | #3
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@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.
> 
> 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.
Christian Eggers Nov. 16, 2020, 9:21 a.m. UTC | #4
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@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.

> > 
> > 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
Vladimir Oltean Nov. 16, 2020, 10:07 a.m. UTC | #5
On Mon, Nov 16, 2020 at 10:21:14AM +0100, Christian Eggers wrote:
> 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@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.

There is a dp->priv that you could use to hold a reference to your
PTP-specific substructure (struct ksz_port_ptp_shared) of
struct ksz_port.

Then, in that PTP-specific per-port substructure, you could hold another
pointer to a common struct ksz_device_ptp_shared.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d173fcbf119..de7e2d80426a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11520,6 +11520,7 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
 F:	drivers/net/dsa/microchip/*
+F:	include/linux/dsa/microchip/ksz_common.h
 F:	include/linux/platform_data/microchip-ksz.h
 F:	net/dsa/tag_ksz.c
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cf866e48ff66..5735374b5bc3 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,92 +7,13 @@ 
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+#include <linux/dsa/ksz_common.h>
 #include <linux/etherdevice.h>
-#include <linux/kernel.h>
-#include <linux/mutex.h>
-#include <linux/phy.h>
-#include <linux/regmap.h>
-#include <net/dsa.h>
 
 struct vlan_table {
 	u32 table[3];
 };
 
-struct ksz_port_mib {
-	struct mutex cnt_mutex;		/* structure access */
-	u8 cnt_ptr;
-	u64 *counters;
-};
-
-struct ksz_port {
-	u16 member;
-	u16 vid_member;
-	int stp_state;
-	struct phy_device phydev;
-
-	u32 on:1;			/* port is not disabled by hardware */
-	u32 phy:1;			/* port has a PHY */
-	u32 fiber:1;			/* port is fiber */
-	u32 sgmii:1;			/* port is SGMII */
-	u32 force:1;
-	u32 read:1;			/* read MIB counters in background */
-	u32 freeze:1;			/* MIB counter freeze is enabled */
-
-	struct ksz_port_mib mib;
-	phy_interface_t interface;
-};
-
-struct ksz_device {
-	struct dsa_switch *ds;
-	struct ksz_platform_data *pdata;
-	const char *name;
-
-	struct mutex dev_mutex;		/* device access */
-	struct mutex regmap_mutex;	/* regmap access */
-	struct mutex alu_mutex;		/* ALU access */
-	struct mutex vlan_mutex;	/* vlan access */
-	const struct ksz_dev_ops *dev_ops;
-
-	struct device *dev;
-	struct regmap *regmap[3];
-
-	void *priv;
-
-	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
-
-	/* chip specific data */
-	u32 chip_id;
-	int num_vlans;
-	int num_alus;
-	int num_statics;
-	int cpu_port;			/* port connected to CPU */
-	int cpu_ports;			/* port bitmap can be cpu port */
-	int phy_port_cnt;
-	int port_cnt;
-	int reg_mib_cnt;
-	int mib_cnt;
-	int mib_port_cnt;
-	int last_port;			/* ports after that not used */
-	phy_interface_t compat_interface;
-	u32 regs_size;
-	bool phy_errata_9477;
-	bool synclko_125;
-
-	struct vlan_table *vlan_cache;
-
-	struct ksz_port *ports;
-	struct delayed_work mib_read;
-	unsigned long mib_read_interval;
-	u16 br_member;
-	u16 member;
-	u16 mirror_rx;
-	u16 mirror_tx;
-	u32 features;			/* chip specific features */
-	u32 overrides;			/* chip functions set by user */
-	u16 host_mask;
-	u16 port_mask;
-};
-
 struct alu_struct {
 	/* entry 1 */
 	u8	is_static:1;
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
new file mode 100644
index 000000000000..3b22380d85c5
--- /dev/null
+++ b/include/linux/dsa/ksz_common.h
@@ -0,0 +1,96 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Included by drivers/net/dsa/microchip/ksz_common.h and net/dsa/tag_ksz.c */
+
+#ifndef _NET_DSA_KSZ_COMMON_H_
+#define _NET_DSA_KSZ_COMMON_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
+#include <net/dsa.h>
+
+struct ksz_platform_data;
+struct ksz_dev_ops;
+struct vlan_table;
+
+struct ksz_port_mib {
+	struct mutex cnt_mutex;		/* structure access */
+	u8 cnt_ptr;
+	u64 *counters;
+};
+
+struct ksz_port {
+	u16 member;
+	u16 vid_member;
+	int stp_state;
+	struct phy_device phydev;
+
+	u32 on:1;			/* port is not disabled by hardware */
+	u32 phy:1;			/* port has a PHY */
+	u32 fiber:1;			/* port is fiber */
+	u32 sgmii:1;			/* port is SGMII */
+	u32 force:1;
+	u32 read:1;			/* read MIB counters in background */
+	u32 freeze:1;			/* MIB counter freeze is enabled */
+
+	struct ksz_port_mib mib;
+	phy_interface_t interface;
+};
+
+struct ksz_device {
+	struct dsa_switch *ds;
+	struct ksz_platform_data *pdata;
+	const char *name;
+
+	struct mutex dev_mutex;		/* device access */
+	struct mutex regmap_mutex;	/* regmap access */
+	struct mutex alu_mutex;		/* ALU access */
+	struct mutex vlan_mutex;	/* vlan access */
+	const struct ksz_dev_ops *dev_ops;
+
+	struct device *dev;
+	struct regmap *regmap[3];
+
+	void *priv;
+
+	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
+
+	/* chip specific data */
+	u32 chip_id;
+	int num_vlans;
+	int num_alus;
+	int num_statics;
+	int cpu_port;			/* port connected to CPU */
+	int cpu_ports;			/* port bitmap can be cpu port */
+	int phy_port_cnt;
+	int port_cnt;
+	int reg_mib_cnt;
+	int mib_cnt;
+	int mib_port_cnt;
+	int last_port;			/* ports after that not used */
+	phy_interface_t compat_interface;
+	u32 regs_size;
+	bool phy_errata_9477;
+	bool synclko_125;
+
+	struct vlan_table *vlan_cache;
+
+	struct ksz_port *ports;
+	struct delayed_work mib_read;
+	unsigned long mib_read_interval;
+	u16 br_member;
+	u16 member;
+	u16 mirror_rx;
+	u16 mirror_tx;
+	u32 features;			/* chip specific features */
+	u32 overrides;			/* chip functions set by user */
+	u16 host_mask;
+	u16 port_mask;
+};
+
+#endif /* _NET_DSA_KSZ_COMMON_H_ */