diff mbox series

[net-next,5/6] net: ti: icss-iep: Move icss_iep structure

Message ID 20240808110800.1281716-6-danishanwar@ti.com (mailing list archive)
State New, archived
Headers show
Series Introduce HSR offload support for ICSSG | expand

Commit Message

MD Danish Anwar Aug. 8, 2024, 11:07 a.m. UTC 
Move icss_iep structure definition and prueth_iep_gettime to icss_iep.h
and icssg_prueth.h file respectively so that they can be used / accessed
by all icssg driver files.

Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icss_iep.c     | 72 -------------------
 drivers/net/ethernet/ti/icssg/icss_iep.h     | 74 +++++++++++++++++++-
 drivers/net/ethernet/ti/icssg/icssg_prueth.c |  5 +-
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 +
 4 files changed, 78 insertions(+), 75 deletions(-)

Comments

Dan Carpenter Aug. 9, 2024, 8:10 p.m. UTC | #1 
On Thu, Aug 08, 2024 at 04:37:59PM +0530, MD Danish Anwar wrote:
> -	struct ptp_clock *ptp_clock;
> -	struct mutex ptp_clk_mutex;	/* PHC access serializer */
> -	u32 def_inc;
> -	s16 slow_cmp_inc;

[ cut ]

> +	struct ptp_clock *ptp_clock;
> +	struct mutex ptp_clk_mutex;	/* PHC access serializer */
> +	spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The patch adds this new struct member.  When you're moving code around, please
just move the code.  Don't fix checkpatch warnings or do any other cleanups.

> +	u32 def_inc;
> +	s16 slow_cmp_inc;
> +	u32 slow_cmp_count;

regards,
dan carpenter
MD Danish Anwar Aug. 12, 2024, 5:41 a.m. UTC | #2 
Hi Dan,

On 10/08/24 1:40 am, Dan Carpenter wrote:
> On Thu, Aug 08, 2024 at 04:37:59PM +0530, MD Danish Anwar wrote:
>> -	struct ptp_clock *ptp_clock;
>> -	struct mutex ptp_clk_mutex;	/* PHC access serializer */
>> -	u32 def_inc;
>> -	s16 slow_cmp_inc;
> 
> [ cut ]
> 
>> +	struct ptp_clock *ptp_clock;
>> +	struct mutex ptp_clk_mutex;	/* PHC access serializer */
>> +	spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> The patch adds this new struct member.  When you're moving code around, please
> just move the code.  Don't fix checkpatch warnings or do any other cleanups.
> 

My bad. I didn't notice this new struct member was introduced. I will
take care of it.

Also apart from doing the code movement, this patch also does the
following change. Instead of hardcoding the value 4, the patch uses
emac->iep->def_inc. Since the iep->def_inc is now accessible from
drivers/net/ethernet/ti/icssg/icssg_prueth.c

@@ -384,7 +384,8 @@ static void prueth_iep_settime(void *clockops_data,
u64 ns)
 	sc_desc.cyclecounter0_set = cyclecount & GENMASK(31, 0);
 	sc_desc.cyclecounter1_set = (cyclecount & GENMASK(63, 32)) >> 32;
 	sc_desc.iepcount_set = ns % cycletime;
-	sc_desc.CMP0_current = cycletime - 4; //Count from 0 to (cycle time)-4
+	/* Count from 0 to (cycle time) - emac->iep->def_inc */
+	sc_desc.CMP0_current = cycletime - emac->iep->def_inc;

 	memcpy_toio(sc_descp, &sc_desc, sizeof(sc_desc));


Should I keep the above change as it is or should I split it into a
separate patch and make this patch strictly for code movement. I kept
the above change as part of this patch because it is related with the
code movement. Moving the iep structure from icss_iep.c to icss_iep.h
makes it accessible to icssg_prueth.c so I thought keeping them together
will be a better idea.

Please let me know if this is okay.

>> +	u32 def_inc;
>> +	s16 slow_cmp_inc;
>> +	u32 slow_cmp_count;
> 
> regards,
> dan carpenter
Dan Carpenter Aug. 12, 2024, 6:48 a.m. UTC | #3 
On Mon, Aug 12, 2024 at 11:11:21AM +0530, MD Danish Anwar wrote:
> Hi Dan,
> 
> On 10/08/24 1:40 am, Dan Carpenter wrote:
> > On Thu, Aug 08, 2024 at 04:37:59PM +0530, MD Danish Anwar wrote:
> >> -	struct ptp_clock *ptp_clock;
> >> -	struct mutex ptp_clk_mutex;	/* PHC access serializer */
> >> -	u32 def_inc;
> >> -	s16 slow_cmp_inc;
> > 
> > [ cut ]
> > 
> >> +	struct ptp_clock *ptp_clock;
> >> +	struct mutex ptp_clk_mutex;	/* PHC access serializer */
> >> +	spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
> >         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > The patch adds this new struct member.  When you're moving code around, please
> > just move the code.  Don't fix checkpatch warnings or do any other cleanups.
> > 
> 
> My bad. I didn't notice this new struct member was introduced. I will
> take care of it.
> 
> Also apart from doing the code movement, this patch also does the
> following change. Instead of hardcoding the value 4, the patch uses
> emac->iep->def_inc. Since the iep->def_inc is now accessible from
> drivers/net/ethernet/ti/icssg/icssg_prueth.c
> 
> @@ -384,7 +384,8 @@ static void prueth_iep_settime(void *clockops_data,
> u64 ns)
>  	sc_desc.cyclecounter0_set = cyclecount & GENMASK(31, 0);
>  	sc_desc.cyclecounter1_set = (cyclecount & GENMASK(63, 32)) >> 32;
>  	sc_desc.iepcount_set = ns % cycletime;
> -	sc_desc.CMP0_current = cycletime - 4; //Count from 0 to (cycle time)-4
> +	/* Count from 0 to (cycle time) - emac->iep->def_inc */
> +	sc_desc.CMP0_current = cycletime - emac->iep->def_inc;
> 
>  	memcpy_toio(sc_descp, &sc_desc, sizeof(sc_desc));
> 
> 
> Should I keep the above change as it is or should I split it into a
> separate patch and make this patch strictly for code movement. I kept
> the above change as part of this patch because it is related with the
> code movement. Moving the iep structure from icss_iep.c to icss_iep.h
> makes it accessible to icssg_prueth.c so I thought keeping them together
> will be a better idea.
> 
> Please let me know if this is okay.
> 

Huh, I saw that the comment had changed but I assumed that was because
checkpatch had complained.  I didn't notice that the 4 changed to
emac->iep->def_inc.  That's the kind of change that we do really want to notice.

Yep.  Please, could you split that out into a separate patch.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
index 75c294ce6fb6..5d6d1cf78e93 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.c
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
@@ -53,78 +53,6 @@ 
 #define IEP_CAP_CFG_CAPNR_1ST_EVENT_EN(n)	BIT(LATCH_INDEX(n))
 #define IEP_CAP_CFG_CAP_ASYNC_EN(n)		BIT(LATCH_INDEX(n) + 10)
 
-enum {
-	ICSS_IEP_GLOBAL_CFG_REG,
-	ICSS_IEP_GLOBAL_STATUS_REG,
-	ICSS_IEP_COMPEN_REG,
-	ICSS_IEP_SLOW_COMPEN_REG,
-	ICSS_IEP_COUNT_REG0,
-	ICSS_IEP_COUNT_REG1,
-	ICSS_IEP_CAPTURE_CFG_REG,
-	ICSS_IEP_CAPTURE_STAT_REG,
-
-	ICSS_IEP_CAP6_RISE_REG0,
-	ICSS_IEP_CAP6_RISE_REG1,
-
-	ICSS_IEP_CAP7_RISE_REG0,
-	ICSS_IEP_CAP7_RISE_REG1,
-
-	ICSS_IEP_CMP_CFG_REG,
-	ICSS_IEP_CMP_STAT_REG,
-	ICSS_IEP_CMP0_REG0,
-	ICSS_IEP_CMP0_REG1,
-	ICSS_IEP_CMP1_REG0,
-	ICSS_IEP_CMP1_REG1,
-
-	ICSS_IEP_CMP8_REG0,
-	ICSS_IEP_CMP8_REG1,
-	ICSS_IEP_SYNC_CTRL_REG,
-	ICSS_IEP_SYNC0_STAT_REG,
-	ICSS_IEP_SYNC1_STAT_REG,
-	ICSS_IEP_SYNC_PWIDTH_REG,
-	ICSS_IEP_SYNC0_PERIOD_REG,
-	ICSS_IEP_SYNC1_DELAY_REG,
-	ICSS_IEP_SYNC_START_REG,
-	ICSS_IEP_MAX_REGS,
-};
-
-/**
- * struct icss_iep_plat_data - Plat data to handle SoC variants
- * @config: Regmap configuration data
- * @reg_offs: register offsets to capture offset differences across SoCs
- * @flags: Flags to represent IEP properties
- */
-struct icss_iep_plat_data {
-	const struct regmap_config *config;
-	u32 reg_offs[ICSS_IEP_MAX_REGS];
-	u32 flags;
-};
-
-struct icss_iep {
-	struct device *dev;
-	void __iomem *base;
-	const struct icss_iep_plat_data *plat_data;
-	struct regmap *map;
-	struct device_node *client_np;
-	unsigned long refclk_freq;
-	int clk_tick_time;	/* one refclk tick time in ns */
-	struct ptp_clock_info ptp_info;
-	struct ptp_clock *ptp_clock;
-	struct mutex ptp_clk_mutex;	/* PHC access serializer */
-	u32 def_inc;
-	s16 slow_cmp_inc;
-	u32 slow_cmp_count;
-	const struct icss_iep_clockops *ops;
-	void *clockops_data;
-	u32 cycle_time_ns;
-	u32 perout_enabled;
-	bool pps_enabled;
-	int cap_cmp_irq;
-	u64 period;
-	u32 latch_enable;
-	struct work_struct work;
-};
-
 /**
  * icss_iep_get_count_hi() - Get the upper 32 bit IEP counter
  * @iep: Pointer to structure representing IEP.
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.h b/drivers/net/ethernet/ti/icssg/icss_iep.h
index 803a4b714893..9283123349e8 100644
--- a/drivers/net/ethernet/ti/icssg/icss_iep.h
+++ b/drivers/net/ethernet/ti/icssg/icss_iep.h
@@ -12,7 +12,79 @@ 
 #include <linux/ptp_clock_kernel.h>
 #include <linux/regmap.h>
 
-struct icss_iep;
+enum {
+	ICSS_IEP_GLOBAL_CFG_REG,
+	ICSS_IEP_GLOBAL_STATUS_REG,
+	ICSS_IEP_COMPEN_REG,
+	ICSS_IEP_SLOW_COMPEN_REG,
+	ICSS_IEP_COUNT_REG0,
+	ICSS_IEP_COUNT_REG1,
+	ICSS_IEP_CAPTURE_CFG_REG,
+	ICSS_IEP_CAPTURE_STAT_REG,
+
+	ICSS_IEP_CAP6_RISE_REG0,
+	ICSS_IEP_CAP6_RISE_REG1,
+
+	ICSS_IEP_CAP7_RISE_REG0,
+	ICSS_IEP_CAP7_RISE_REG1,
+
+	ICSS_IEP_CMP_CFG_REG,
+	ICSS_IEP_CMP_STAT_REG,
+	ICSS_IEP_CMP0_REG0,
+	ICSS_IEP_CMP0_REG1,
+	ICSS_IEP_CMP1_REG0,
+	ICSS_IEP_CMP1_REG1,
+
+	ICSS_IEP_CMP8_REG0,
+	ICSS_IEP_CMP8_REG1,
+	ICSS_IEP_SYNC_CTRL_REG,
+	ICSS_IEP_SYNC0_STAT_REG,
+	ICSS_IEP_SYNC1_STAT_REG,
+	ICSS_IEP_SYNC_PWIDTH_REG,
+	ICSS_IEP_SYNC0_PERIOD_REG,
+	ICSS_IEP_SYNC1_DELAY_REG,
+	ICSS_IEP_SYNC_START_REG,
+	ICSS_IEP_MAX_REGS,
+};
+
+/**
+ * struct icss_iep_plat_data - Plat data to handle SoC variants
+ * @config: Regmap configuration data
+ * @reg_offs: register offsets to capture offset differences across SoCs
+ * @flags: Flags to represent IEP properties
+ */
+struct icss_iep_plat_data {
+	const struct regmap_config *config;
+	u32 reg_offs[ICSS_IEP_MAX_REGS];
+	u32 flags;
+};
+
+struct icss_iep {
+	struct device *dev;
+	void __iomem *base;
+	const struct icss_iep_plat_data *plat_data;
+	struct regmap *map;
+	struct device_node *client_np;
+	unsigned long refclk_freq;
+	int clk_tick_time;	/* one refclk tick time in ns */
+	struct ptp_clock_info ptp_info;
+	struct ptp_clock *ptp_clock;
+	struct mutex ptp_clk_mutex;	/* PHC access serializer */
+	spinlock_t irq_lock; /* CMP IRQ vs icss_iep_ptp_enable access */
+	u32 def_inc;
+	s16 slow_cmp_inc;
+	u32 slow_cmp_count;
+	const struct icss_iep_clockops *ops;
+	void *clockops_data;
+	u32 cycle_time_ns;
+	u32 perout_enabled;
+	bool pps_enabled;
+	int cap_cmp_irq;
+	u64 period;
+	u32 latch_enable;
+	struct work_struct work;
+};
+
 extern const struct icss_iep_clockops prueth_iep_clockops;
 
 /* Firmware specific clock operations */
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 85c9797b6238..778b41c16f2d 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -325,7 +325,7 @@  static int emac_phy_connect(struct prueth_emac *emac)
 	return 0;
 }
 
-static u64 prueth_iep_gettime(void *clockops_data, struct ptp_system_timestamp *sts)
+u64 prueth_iep_gettime(void *clockops_data, struct ptp_system_timestamp *sts)
 {
 	u32 hi_rollover_count, hi_rollover_count_r;
 	struct prueth_emac *emac = clockops_data;
@@ -384,7 +384,8 @@  static void prueth_iep_settime(void *clockops_data, u64 ns)
 	sc_desc.cyclecounter0_set = cyclecount & GENMASK(31, 0);
 	sc_desc.cyclecounter1_set = (cyclecount & GENMASK(63, 32)) >> 32;
 	sc_desc.iepcount_set = ns % cycletime;
-	sc_desc.CMP0_current = cycletime - 4; //Count from 0 to (cycle time)-4
+	/* Count from 0 to (cycle time) - emac->iep->def_inc */
+	sc_desc.CMP0_current = cycletime - emac->iep->def_inc;
 
 	memcpy_toio(sc_descp, &sc_desc, sizeof(sc_desc));
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 6cb1dce8b309..79e8577caf91 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -362,6 +362,8 @@  void icssg_stats_work_handler(struct work_struct *work);
 void emac_update_hardware_stats(struct prueth_emac *emac);
 int emac_get_stat_by_name(struct prueth_emac *emac, char *stat_name);
 
+u64 prueth_iep_gettime(void *clockops_data, struct ptp_system_timestamp *sts);
+
 /* Common functions */
 void prueth_cleanup_rx_chns(struct prueth_emac *emac,
 			    struct prueth_rx_chn *rx_chn,