On 1/24/22 5:46 PM, Thinh Nguyen wrote: > Sean Anderson wrote: >> GUCTL.REFCLKPER can only account for clock frequencies with integer >> periods. To address this, program REFCLK_FLADJ with the relative error >> caused by period truncation. The formula given in the register reference >> has been rearranged to allow calculation based on rate (instead of >> period), and to allow for fixed-point arithmetic. >> >> Additionally, calculate a value for 240MHZDECR. This configures a >> simulated 240Mhz clock using a counter with one fractional bit (PLS1). >> >> This register is programmed only for versions >= 2.50a, since this is >> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length >> adjustment quirk"). >> >> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx> >> --- >> >> Changes in v2: >> - Also program GFLADJ.240MHZDECR >> - Don't program GFLADJ if the version is < 2.50a >> >> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++-- >> drivers/usb/dwc3/core.h | 3 +++ >> 2 files changed, 38 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 5214daceda86..883e119377f0 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc) >> static void dwc3_ref_clk_period(struct dwc3 *dwc) >> { >> u32 reg; >> - unsigned long rate, period; >> + unsigned long decr, fladj, rate, period; > > Minor style nit: Felipe prefers to keep the declaration on separate > lines, let's keep it consistent with the rest in this driver. So unsigned int decr; unsigned int fladj; unsigned int rate; unsigned int period; ? Frankly that seems rather verbose. >> >> if (dwc->ref_clk) { >> rate = clk_get_rate(dwc->ref_clk); >> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc) >> period = NSEC_PER_SEC / rate; >> } else if (dwc->ref_clk_per) { >> period = dwc->ref_clk_per; >> + rate = NSEC_PER_SEC / period; >> } else { >> return; >> } >> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc) >> reg &= ~DWC3_GUCTL_REFCLKPER_MASK; >> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period); >> dwc3_writel(dwc->regs, DWC3_GUCTL, reg); >> + >> + if (DWC3_VER_IS_PRIOR(DWC3, 250A)) >> + return; >> + >> + /* >> + * The calculation below is >> + * >> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1) >> + * >> + * but rearranged for fixed-point arithmetic. >> + * >> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of >> + * nanoseconds of error caused by the truncation which happened during >> + * the division when calculating rate or period (whichever one was >> + * derived from the other). We first calculate the relative error, then >> + * scale it to units of 0.08%. >> + */ >> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period); > > Can we rearrange the math so we don't have to operate on different data > type and deal with conversion/truncation? I don't understand what data types you are referring to. The truncation above (in the calculaion for rate/period) is intentional, so we can determine the error introduced by representing period using only ns. >> + fladj -= 125000; >> + >> + /* >> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well. >> + */ >> + decr = 480000000 / rate; >> + >> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ); >> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK >> + & ~DWC3_GFLADJ_240MHZDECR >> + & ~DWC3_GFLADJ_240MHZDECR_PLS1; >> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj) >> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1) >> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1); > > Does this pass checkpatch? Yes. --Sean >> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg); >> } >> >> - >> /** >> * dwc3_free_one_event_buffer - Frees one event buffer >> * @dwc: Pointer to our controller context structure >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 45cfa7d9f27a..eb9c1efced05 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -388,6 +388,9 @@ >> /* Global Frame Length Adjustment Register */ >> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7) >> #define DWC3_GFLADJ_30MHZ_MASK 0x3f >> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8) >> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24) >> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31) >> >> /* Global User Control Register*/ >> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000 > Thanks, > Thinh >