| Message ID | 20250211103527.923849-2-m-malladi@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for perout configuration in IEP driver | expand |
On 2/11/25 11:35 AM, Meghana Malladi wrote: > @@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, > regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); > if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) > regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp)); > - /* Configure SYNC, 1ms pulse width */ > - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000); > + /* Configure SYNC, based on req on width */ > + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, > + (u32)(ns_width / iep->def_inc)); This causes build errors on 32bits: ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] undefined! make[3]: *** [../scripts/Makefile.modpost:147: Module.symvers] Error 1 make[2]: *** [/home/nipa/net/wt-0/Makefile:1944: modpost] Error 2 make[1]: *** [/home/nipa/net/wt-0/Makefile:251: __sub-make] Error 2 make: *** [Makefile:251: __sub-make] Error 2 ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] You should use div_u64() /P
On 2/13/2025 4:53 PM, Paolo Abeni wrote: > On 2/11/25 11: 35 AM, Meghana Malladi wrote: > @@ -419,8 +426,9 @@ > static int icss_iep_perout_enable_hw(struct icss_iep *iep, > > regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); > if > (iep->plat_data->flags & > ZjQcmQRYFpfptBannerStart > This message was sent from outside of Texas Instruments. > Do not click links or open attachments unless you recognize the source > of this email and know the content is safe. > Report Suspicious > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! > updqdzavl0dbisXOnfkDHxHqGlQUHEro3tgnljLa7x4DRPBIRKu8Nqm3bW1LeMtXFyqz6yM7_tLlrvUmslKj9m_IL0hUlNU$> > ZjQcmQRYFpfptBannerEnd > > On 2/11/25 11:35 AM, Meghana Malladi wrote: >> @@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, >> regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); >> if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) >> regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp)); >> - /* Configure SYNC, 1ms pulse width */ >> - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000); >> + /* Configure SYNC, based on req on width */ >> + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, >> + (u32)(ns_width / iep->def_inc)); > > This causes build errors on 32bits: > > ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] > undefined! > make[3]: *** [../scripts/Makefile.modpost:147: Module.symvers] Error 1 > make[2]: *** [/home/nipa/net/wt-0/Makefile:1944: modpost] Error 2 > make[1]: *** [/home/nipa/net/wt-0/Makefile:251: __sub-make] Error 2 > make: *** [Makefile:251: __sub-make] Error 2 > ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] > > You should use div_u64() > I see, thanks. Can you tell me how can I reproduce this on my end for 32 bits. Will fix this in v2. > /P >
On Fri, Feb 14, 2025 at 11:35:07AM +0530, Malladi, Meghana wrote: > > > On 2/13/2025 4:53 PM, Paolo Abeni wrote: > > On 2/11/25 11: 35 AM, Meghana Malladi wrote: > @@ -419,8 +426,9 @@ > > static int icss_iep_perout_enable_hw(struct icss_iep *iep, > > > regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); > if > > (iep->plat_data->flags & > > ZjQcmQRYFpfptBannerStart > > This message was sent from outside of Texas Instruments. > > Do not click links or open attachments unless you recognize the source > > of this email and know the content is safe. > > Report Suspicious > > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! updqdzavl0dbisXOnfkDHxHqGlQUHEro3tgnljLa7x4DRPBIRKu8Nqm3bW1LeMtXFyqz6yM7_tLlrvUmslKj9m_IL0hUlNU$> > > ZjQcmQRYFpfptBannerEnd > > > > On 2/11/25 11:35 AM, Meghana Malladi wrote: > > > @@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, > > > regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); > > > if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) > > > regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp)); > > > - /* Configure SYNC, 1ms pulse width */ > > > - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000); > > > + /* Configure SYNC, based on req on width */ > > > + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, > > > + (u32)(ns_width / iep->def_inc)); > > > > This causes build errors on 32bits: > > > > ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] > > undefined! > > make[3]: *** [../scripts/Makefile.modpost:147: Module.symvers] Error 1 > > make[2]: *** [/home/nipa/net/wt-0/Makefile:1944: modpost] Error 2 > > make[1]: *** [/home/nipa/net/wt-0/Makefile:251: __sub-make] Error 2 > > make: *** [Makefile:251: __sub-make] Error 2 > > ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] > > > > You should use div_u64() > > > > I see, thanks. > Can you tell me how can I reproduce this on my end for 32 bits. > Will fix this in v2. Hi Meghana, FWIIW, I was able to reproduce this problem running the following on an x86_64 system: ARCH=i386 make tinyconfig echo CONFIG_COMPILE_TEST=y >> .config echo CONFIG_PCI=y >> .config echo CONFIG_SOC_TI=y >> .config echo CONFIG_TI_PRUSS=y >> .config echo CONFIG_NET=y >> .config echo CONFIG_NETDEVICES=y >> .config yes "" | ARCH=i386 make oldconfig ARCH=i386 make -j$(nproc)
Hi Simon On 2/16/2025 9:41 PM, Simon Horman wrote: > On Fri, Feb 14, 2025 at 11:35:07AM +0530, Malladi, Meghana wrote: >> >> >> On 2/13/2025 4:53 PM, Paolo Abeni wrote: >>> On 2/11/25 11: 35 AM, Meghana Malladi wrote: > @@ -419,8 +426,9 @@ >>> static int icss_iep_perout_enable_hw(struct icss_iep *iep, > >>> regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); > if >>> (iep->plat_data->flags & >>> ZjQcmQRYFpfptBannerStart >>> This message was sent from outside of Texas Instruments. >>> Do not click links or open attachments unless you recognize the source >>> of this email and know the content is safe. >>> Report Suspicious >>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK! updqdzavl0dbisXOnfkDHxHqGlQUHEro3tgnljLa7x4DRPBIRKu8Nqm3bW1LeMtXFyqz6yM7_tLlrvUmslKj9m_IL0hUlNU$> >>> ZjQcmQRYFpfptBannerEnd >>> >>> On 2/11/25 11:35 AM, Meghana Malladi wrote: >>>> @@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, >>>> regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); >>>> if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) >>>> regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp)); >>>> - /* Configure SYNC, 1ms pulse width */ >>>> - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000); >>>> + /* Configure SYNC, based on req on width */ >>>> + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, >>>> + (u32)(ns_width / iep->def_inc)); >>> >>> This causes build errors on 32bits: >>> >>> ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] >>> undefined! >>> make[3]: *** [../scripts/Makefile.modpost:147: Module.symvers] Error 1 >>> make[2]: *** [/home/nipa/net/wt-0/Makefile:1944: modpost] Error 2 >>> make[1]: *** [/home/nipa/net/wt-0/Makefile:251: __sub-make] Error 2 >>> make: *** [Makefile:251: __sub-make] Error 2 >>> ERROR: modpost: "__udivdi3" [drivers/net/ethernet/ti/icssg/icss_iep.ko] >>> >>> You should use div_u64() >>> >> >> I see, thanks. >> Can you tell me how can I reproduce this on my end for 32 bits. >> Will fix this in v2. > > Hi Meghana, > > FWIIW, I was able to reproduce this problem running the following > on an x86_64 system: > > ARCH=i386 make tinyconfig > > echo CONFIG_COMPILE_TEST=y >> .config > echo CONFIG_PCI=y >> .config > echo CONFIG_SOC_TI=y >> .config > echo CONFIG_TI_PRUSS=y >> .config > echo CONFIG_NET=y >> .config > echo CONFIG_NETDEVICES=y >> .config > > yes "" | ARCH=i386 make oldconfig > > ARCH=i386 make -j$(nproc) Thanks for sharing this, I was able to reproduce and fix this locally.
diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c index 768578c0d958..789516683ca9 100644 --- a/drivers/net/ethernet/ti/icssg/icss_iep.c +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c @@ -406,9 +406,16 @@ static void icss_iep_update_to_next_boundary(struct icss_iep *iep, u64 start_ns) static int icss_iep_perout_enable_hw(struct icss_iep *iep, struct ptp_perout_request *req, int on) { + struct timespec64 ts; + u64 ns_width; int ret; u64 cmp; + /* Calculate width of the signal for PPS/PEROUT handling */ + ts.tv_sec = req->on.sec; + ts.tv_nsec = req->on.nsec; + ns_width = timespec64_to_ns(&ts); + if (iep->ops && iep->ops->perout_enable) { ret = iep->ops->perout_enable(iep->clockops_data, req, on, &cmp); if (ret) @@ -419,8 +426,9 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp)); if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT) regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp)); - /* Configure SYNC, 1ms pulse width */ - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, 1000000); + /* Configure SYNC, based on req on width */ + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, + (u32)(ns_width / iep->def_inc)); regmap_write(iep->map, ICSS_IEP_SYNC0_PERIOD_REG, 0); regmap_write(iep->map, ICSS_IEP_SYNC_START_REG, 0); regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, 0); /* one-shot mode */ @@ -447,6 +455,8 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep, + req->period.nsec; icss_iep_update_to_next_boundary(iep, start_ns); + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG, + (u32)(ns_width / iep->def_inc)); /* Enable Sync in single shot mode */ regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, IEP_SYNC_CTRL_SYNC_N_EN(0) | IEP_SYNC_CTRL_SYNC_EN); @@ -478,6 +488,12 @@ static int icss_iep_perout_enable(struct icss_iep *iep, mutex_lock(&iep->ptp_clk_mutex); + /* Reject requests with unsupported flags */ + if (req->flags & ~PTP_PEROUT_DUTY_CYCLE) { + ret = -EOPNOTSUPP; + goto exit; + } + if (iep->pps_enabled) { ret = -EBUSY; goto exit; @@ -486,6 +502,12 @@ static int icss_iep_perout_enable(struct icss_iep *iep, if (iep->perout_enabled == !!on) goto exit; + /* Set default "on" time (1ms) for the signal if not passed by the app */ + if (!(req->flags & PTP_PEROUT_DUTY_CYCLE)) { + req->on.sec = 0; + req->on.nsec = NSEC_PER_MSEC; + } + ret = icss_iep_perout_enable_hw(iep, req, on); if (!ret) iep->perout_enabled = !!on; @@ -572,6 +594,8 @@ static int icss_iep_pps_enable(struct icss_iep *iep, int on) rq.perout.period.nsec = 0; rq.perout.start.sec = ts.tv_sec + 2; rq.perout.start.nsec = 0; + rq.perout.on.sec = 0; + rq.perout.on.nsec = NSEC_PER_MSEC; ret = icss_iep_perout_enable_hw(iep, &rq.perout, on); } else { ret = icss_iep_perout_enable_hw(iep, &rq.perout, on);
icss_iep_perout_enable_hw() is a common function for generating both pps and perout signals. When enabling pps, the application needs to only pass enable/disable argument, whereas for perout it supports different flags to configure the signal. But icss_iep_perout_enable_hw() function is missing to hook the configuration params passed by the app, causing perout to behave same a pps (except being able to configure the period). As duty cycle is also one feature which can configured for perout, incorporate this in the function to get the expected signal. Fixes: c1e0230eeaab2 ("net: ti: icss-iep: Add IEP driver") Signed-off-by: Meghana Malladi <m-malladi@ti.com> --- drivers/net/ethernet/ti/icssg/icss_iep.c | 28 ++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)