Sean Anderson wrote: > > > On 1/24/22 9:11 PM, Thinh Nguyen wrote: >> Sean Anderson wrote: >>> >>> >>> 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. >> >> A couple of the benefits of having it like this is to help with viewing >> git-blame if we introduce new variables and help with backporting fix >> patch a bit simpler. Mainly I'm just following Felipe's style and keep >> it consistent in this driver, but I don't think it's a big deal. > > *shrug* > > If it's the subsystem style I will rewrite it. > Felipe also prefers Reverse Christmas Tree style. > (btw is this documented anywhere for future contributors?) > I don't think it's documented, but Felipe Nak'ed patches that don't follow this style before. I don't want this to be the main focus of the conversation for this patch, but I don't want it to go unnoticed either. Your concern is not alone regarding document (or the lacking of) for different subsystem-specific styles. (see https://lwn.net/Articles/758552/) >>> >>>>> >>>>> 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. >> >> I was wondering if we rearrange the math so we don't need to cast and >> use 64-bit here, but that may not be possible. Just computing/reviewing >> in my head while accounting for truncation from 64-bit to 32-bit value >> is taxing. > > Well the primary issue is that log_2(125000ULL * NSEC_PER_SEC) ~= 49, so > we have to use 64-bit integers if we don't want to do any shifting of > the numerator. It might be possible to analyze the significant bits > through the calculation and determine that we can use less precision for > some of the intermediate calculations, but I haven't done that analysis. > IMO that sort of transformation would likely make the calculations more > difficult to understand, not less. Fair enough. Thanks, Thinh