| 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 |
| 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 |
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.
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".
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.
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
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 --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_ */
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