| Message ID | 20240808110800.1281716-6-danishanwar@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce HSR offload support for ICSSG | expand |
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
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
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 --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,