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
>
