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
