diff mbox series

[v2,2/7] net: dsa: b53: Move struct b53_device to include/linux/dsa/b53.h

Message ID 20211109095013.27829-3-martin.kaistra@linutronix.de (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series Add PTP support for BCM53128 switch | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning CHECK: struct mutex definition without comment WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Martin Kaistra Nov. 9, 2021, 9:50 a.m. UTC 
In order to access the b53 structs from net/dsa/tag_brcm.c move the
definitions from drivers/net/dsa/b53/b53_priv.h to the new file
include/linux/dsa/b53.h.

Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
---
 drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
 include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 89 deletions(-)
 create mode 100644 include/linux/dsa/b53.h

Comments

Florian Fainelli Nov. 9, 2021, 6:05 p.m. UTC | #1 
On 11/9/21 1:50 AM, Martin Kaistra wrote:
> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> include/linux/dsa/b53.h.
> 
> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> ---
>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+), 89 deletions(-)
>  create mode 100644 include/linux/dsa/b53.h

All you really access is the b53_port_hwtstamp structure within the
tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
Florian Fainelli Nov. 9, 2021, 6:11 p.m. UTC | #2 
On 11/9/21 10:05 AM, Florian Fainelli wrote:
> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
>> include/linux/dsa/b53.h.
>>
>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
>> ---
>>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
>>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
>>  2 files changed, 101 insertions(+), 89 deletions(-)
>>  create mode 100644 include/linux/dsa/b53.h
> 
> All you really access is the b53_port_hwtstamp structure within the
> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.

You do access b53_dev in the TX part, still, I would like to find a more
elegant solution to exposing everything here, can you create a
b53_timecounter_cyc2time() function that is exported to modules but does
not require exposing the b53_device to net/dsa/tag_brcm.c?
Vladimir Oltean Nov. 9, 2021, 6:15 p.m. UTC | #3 
On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 11/9/21 10:05 AM, Florian Fainelli wrote:
> > On 11/9/21 1:50 AM, Martin Kaistra wrote:
> >> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> >> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> >> include/linux/dsa/b53.h.
> >>
> >> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> >> ---
> >>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
> >>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 101 insertions(+), 89 deletions(-)
> >>  create mode 100644 include/linux/dsa/b53.h
> >
> > All you really access is the b53_port_hwtstamp structure within the
> > tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
>
> You do access b53_dev in the TX part, still, I would like to find a more
> elegant solution to exposing everything here, can you create a
> b53_timecounter_cyc2time() function that is exported to modules but does
> not require exposing the b53_device to net/dsa/tag_brcm.c?
> --
> Florian

Switch drivers can't export symbols to tagging protocol drivers, remember?
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
Florian Fainelli Nov. 9, 2021, 6:20 p.m. UTC | #4 
On 11/9/21 10:15 AM, Vladimir Oltean wrote:
> On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 11/9/21 10:05 AM, Florian Fainelli wrote:
>>> On 11/9/21 1:50 AM, Martin Kaistra wrote:
>>>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
>>>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
>>>> include/linux/dsa/b53.h.
>>>>
>>>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
>>>> ---
>>>>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
>>>>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
>>>>  2 files changed, 101 insertions(+), 89 deletions(-)
>>>>  create mode 100644 include/linux/dsa/b53.h
>>>
>>> All you really access is the b53_port_hwtstamp structure within the
>>> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
>>
>> You do access b53_dev in the TX part, still, I would like to find a more
>> elegant solution to exposing everything here, can you create a
>> b53_timecounter_cyc2time() function that is exported to modules but does
>> not require exposing the b53_device to net/dsa/tag_brcm.c?
>> --
>> Florian
> 
> Switch drivers can't export symbols to tagging protocol drivers, remember?
> https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

I do now :) How about a function pointer in dsa_switch_ops that driver
can hook onto?
Vladimir Oltean Nov. 9, 2021, 6:49 p.m. UTC | #5 
On Tue, Nov 09, 2021 at 10:20:25AM -0800, Florian Fainelli wrote:
> On 11/9/21 10:15 AM, Vladimir Oltean wrote:
> > On Tue, 9 Nov 2021 at 20:11, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> On 11/9/21 10:05 AM, Florian Fainelli wrote:
> >>> On 11/9/21 1:50 AM, Martin Kaistra wrote:
> >>>> In order to access the b53 structs from net/dsa/tag_brcm.c move the
> >>>> definitions from drivers/net/dsa/b53/b53_priv.h to the new file
> >>>> include/linux/dsa/b53.h.
> >>>>
> >>>> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> >>>> ---
> >>>>  drivers/net/dsa/b53/b53_priv.h |  90 +----------------------------
> >>>>  include/linux/dsa/b53.h        | 100 +++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 101 insertions(+), 89 deletions(-)
> >>>>  create mode 100644 include/linux/dsa/b53.h
> >>>
> >>> All you really access is the b53_port_hwtstamp structure within the
> >>> tagger, so please make it the only structure exposed to net/dsa/tag_brcm.c.
> >>
> >> You do access b53_dev in the TX part, still, I would like to find a more
> >> elegant solution to exposing everything here, can you create a
> >> b53_timecounter_cyc2time() function that is exported to modules but does
> >> not require exposing the b53_device to net/dsa/tag_brcm.c?
> >> --
> >> Florian
> > 
> > Switch drivers can't export symbols to tagging protocol drivers, remember?
> > https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/
> 
> I do now :) How about a function pointer in dsa_switch_ops that driver
> can hook onto?

IMHO, the honest answer to this is to admit that it's not quite okay to
timestamp a single packet at a time and simply not timestamp any packet
that might be concurrent with that, no warning or questions asked. We
should queue that second packet if it cannot be marked for timestamping
right away.

Which brings me to my second point, there used to be a generic deferred
xmit mechanism in DSA that also happened to solve this problem because
yes, there was a function pointer in dsa_switch_ops for it.
But you and Richard didn't quite like it, so I removed it.
https://patchwork.ozlabs.org/cover/1215617/
diff mbox series

Patch

diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 579da74ada64..1652e489b737 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -23,44 +23,13 @@ 
 #include <linux/mutex.h>
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
-#include <net/dsa.h>
+#include <linux/dsa/b53.h>
 
 #include "b53_regs.h"
 
-struct b53_device;
 struct net_device;
 struct phylink_link_state;
 
-struct b53_io_ops {
-	int (*read8)(struct b53_device *dev, u8 page, u8 reg, u8 *value);
-	int (*read16)(struct b53_device *dev, u8 page, u8 reg, u16 *value);
-	int (*read32)(struct b53_device *dev, u8 page, u8 reg, u32 *value);
-	int (*read48)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
-	int (*read64)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
-	int (*write8)(struct b53_device *dev, u8 page, u8 reg, u8 value);
-	int (*write16)(struct b53_device *dev, u8 page, u8 reg, u16 value);
-	int (*write32)(struct b53_device *dev, u8 page, u8 reg, u32 value);
-	int (*write48)(struct b53_device *dev, u8 page, u8 reg, u64 value);
-	int (*write64)(struct b53_device *dev, u8 page, u8 reg, u64 value);
-	int (*phy_read16)(struct b53_device *dev, int addr, int reg, u16 *value);
-	int (*phy_write16)(struct b53_device *dev, int addr, int reg, u16 value);
-	int (*irq_enable)(struct b53_device *dev, int port);
-	void (*irq_disable)(struct b53_device *dev, int port);
-	u8 (*serdes_map_lane)(struct b53_device *dev, int port);
-	int (*serdes_link_state)(struct b53_device *dev, int port,
-				 struct phylink_link_state *state);
-	void (*serdes_config)(struct b53_device *dev, int port,
-			      unsigned int mode,
-			      const struct phylink_link_state *state);
-	void (*serdes_an_restart)(struct b53_device *dev, int port);
-	void (*serdes_link_set)(struct b53_device *dev, int port,
-				unsigned int mode, phy_interface_t interface,
-				bool link_up);
-	void (*serdes_phylink_validate)(struct b53_device *dev, int port,
-					unsigned long *supported,
-					struct phylink_link_state *state);
-};
-
 #define B53_INVALID_LANE	0xff
 
 enum {
@@ -89,63 +58,6 @@  enum {
 #define B53_N_PORTS	9
 #define B53_N_PORTS_25	6
 
-struct b53_port {
-	u16		vlan_ctl_mask;
-	struct ethtool_eee eee;
-};
-
-struct b53_vlan {
-	u16 members;
-	u16 untag;
-	bool valid;
-};
-
-struct b53_device {
-	struct dsa_switch *ds;
-	struct b53_platform_data *pdata;
-	const char *name;
-
-	struct mutex reg_mutex;
-	struct mutex stats_mutex;
-	struct mutex arl_mutex;
-	const struct b53_io_ops *ops;
-
-	/* chip specific data */
-	u32 chip_id;
-	u8 core_rev;
-	u8 vta_regs[3];
-	u8 duplex_reg;
-	u8 jumbo_pm_reg;
-	u8 jumbo_size_reg;
-	int reset_gpio;
-	u8 num_arl_bins;
-	u16 num_arl_buckets;
-	enum dsa_tag_protocol tag_protocol;
-
-	/* used ports mask */
-	u16 enabled_ports;
-	unsigned int imp_port;
-
-	/* connect specific data */
-	u8 current_page;
-	struct device *dev;
-	u8 serdes_lane;
-
-	/* Master MDIO bus we got probed from */
-	struct mii_bus *bus;
-
-	void *priv;
-
-	/* run time configuration */
-	bool enable_jumbo;
-
-	unsigned int num_vlans;
-	struct b53_vlan *vlans;
-	bool vlan_enabled;
-	unsigned int num_ports;
-	struct b53_port *ports;
-};
-
 #define b53_for_each_port(dev, i) \
 	for (i = 0; i < B53_N_PORTS; i++) \
 		if (dev->enabled_ports & BIT(i))
diff --git a/include/linux/dsa/b53.h b/include/linux/dsa/b53.h
new file mode 100644
index 000000000000..af782a1da362
--- /dev/null
+++ b/include/linux/dsa/b53.h
@@ -0,0 +1,100 @@ 
+/* SPDX-License-Identifier: ISC */
+/*
+ * Copyright (C) 2011-2013 Jonas Gorski <jogo@openwrt.org>
+ *
+ * Included by drivers/net/dsa/b53/b53_priv.h and net/dsa/tag_brcm.c
+ */
+
+#include <net/dsa.h>
+
+struct b53_device;
+struct phylink_link_state;
+
+struct b53_io_ops {
+	int (*read8)(struct b53_device *dev, u8 page, u8 reg, u8 *value);
+	int (*read16)(struct b53_device *dev, u8 page, u8 reg, u16 *value);
+	int (*read32)(struct b53_device *dev, u8 page, u8 reg, u32 *value);
+	int (*read48)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
+	int (*read64)(struct b53_device *dev, u8 page, u8 reg, u64 *value);
+	int (*write8)(struct b53_device *dev, u8 page, u8 reg, u8 value);
+	int (*write16)(struct b53_device *dev, u8 page, u8 reg, u16 value);
+	int (*write32)(struct b53_device *dev, u8 page, u8 reg, u32 value);
+	int (*write48)(struct b53_device *dev, u8 page, u8 reg, u64 value);
+	int (*write64)(struct b53_device *dev, u8 page, u8 reg, u64 value);
+	int (*phy_read16)(struct b53_device *dev, int addr, int reg,
+			  u16 *value);
+	int (*phy_write16)(struct b53_device *dev, int addr, int reg,
+			   u16 value);
+	int (*irq_enable)(struct b53_device *dev, int port);
+	void (*irq_disable)(struct b53_device *dev, int port);
+	u8 (*serdes_map_lane)(struct b53_device *dev, int port);
+	int (*serdes_link_state)(struct b53_device *dev, int port,
+				 struct phylink_link_state *state);
+	void (*serdes_config)(struct b53_device *dev, int port,
+			      unsigned int mode,
+			      const struct phylink_link_state *state);
+	void (*serdes_an_restart)(struct b53_device *dev, int port);
+	void (*serdes_link_set)(struct b53_device *dev, int port,
+				unsigned int mode, phy_interface_t interface,
+				bool link_up);
+	void (*serdes_phylink_validate)(struct b53_device *dev, int port,
+					unsigned long *supported,
+					struct phylink_link_state *state);
+};
+
+struct b53_port {
+	u16 vlan_ctl_mask;
+	struct ethtool_eee eee;
+};
+
+struct b53_vlan {
+	u16 members;
+	u16 untag;
+	bool valid;
+};
+
+struct b53_device {
+	struct dsa_switch *ds;
+	struct b53_platform_data *pdata;
+	const char *name;
+
+	struct mutex reg_mutex;
+	struct mutex stats_mutex;
+	struct mutex arl_mutex;
+	const struct b53_io_ops *ops;
+
+	/* chip specific data */
+	u32 chip_id;
+	u8 core_rev;
+	u8 vta_regs[3];
+	u8 duplex_reg;
+	u8 jumbo_pm_reg;
+	u8 jumbo_size_reg;
+	int reset_gpio;
+	u8 num_arl_bins;
+	u16 num_arl_buckets;
+	enum dsa_tag_protocol tag_protocol;
+
+	/* used ports mask */
+	u16 enabled_ports;
+	unsigned int imp_port;
+
+	/* connect specific data */
+	u8 current_page;
+	struct device *dev;
+	u8 serdes_lane;
+
+	/* Master MDIO bus we got probed from */
+	struct mii_bus *bus;
+
+	void *priv;
+
+	/* run time configuration */
+	bool enable_jumbo;
+
+	unsigned int num_vlans;
+	struct b53_vlan *vlans;
+	bool vlan_enabled;
+	unsigned int num_ports;
+	struct b53_port *ports;
+};