| Message ID | 20220119002438.106079-5-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: Calculate REFCLKPER et. al. from reference clock | expand |
On Tue, 2022-01-18 at 19:24 -0500, 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@seco.com> > --- > > 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; > > 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); > + 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); > + 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 Looks good here on a ZynqMP board. GFLADJ_REFCLK_FLADJ ends up being set to 1 now instead of 0, since the clock rate is actually 19999800 Hz rather than 20 MHz. But that should be correct, and USB still seems to work fine. Reviewed-by: Robert Hancock <robert.hancock@calian.com> Tested-by: Robert Hancock <robert.hancock@calian.com>
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@seco.com> > --- > > 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. > > 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? > + 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? > + 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
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@seco.com> >> --- >> >> 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 >
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@seco.com> >>> --- >>> >>> 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. > >>> >>> 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. > >>> + 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
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> On 1/24/22 5:46 PM, Thinh Nguyen wrote: >>> Sean Anderson wrote: >>>> 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 it also prevents single lines from being constantly modified just because we're adding a new variable. In the diff, adding a new variable should look like a new line being added, rather than modified. > patch a bit simpler. Mainly I'm just following Felipe's style and keep it's part of the kernel coding style, actually :-)
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@seco.com> >>>> --- >>>> >>>> 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. (btw is this documented anywhere for future contributors?) >> >>>> >>>> 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. >> >>>> + 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 >
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@seco.com> >>>>> --- >>>>> >>>>> 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
Felipe Balbi wrote: > > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> On 1/24/22 5:46 PM, Thinh Nguyen wrote: >>>> Sean Anderson wrote: >>>>> 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 > > it also prevents single lines from being constantly modified just > because we're adding a new variable. In the diff, adding a new variable > should look like a new line being added, rather than modified. > Right. >> patch a bit simpler. Mainly I'm just following Felipe's style and keep > > it's part of the kernel coding style, actually :-) > Glad to see you're here :) BR, Thinh
Hi, Sean Anderson <sean.anderson@seco.com> writes: > On 1/24/22 9:11 PM, Thinh Nguyen wrote: >>>>> 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. > > (btw is this documented anywhere for future contributors?) https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting "To this end, use just one data declaration per line (no commas for multiple data declarations)"
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> 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@seco.com> >>>>>> --- >>>>>> >>>>>> 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. that's also part of the coding style and probably documented somewhere
On 1/26/22 5:53 AM, Felipe Balbi wrote: > > Hi, > > Sean Anderson <sean.anderson@seco.com> writes: > >> On 1/24/22 9:11 PM, Thinh Nguyen wrote: >>>>>> 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. >> >> (btw is this documented anywhere for future contributors?) > > https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting > > "To this end, use just one data declaration per line (no commas for > multiple data declarations)" > This is just if you want to add comments. --Sean
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; 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); + 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); + 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
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@seco.com> --- 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(-)