Message-ID: <524027b7-1361-7cc2-842f-7c20cd4cb016@synopsys.com>
Date:   Tue, 25 Jan 2022 19:36:04 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Sean Anderson <sean.anderson@...o.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
CC:     Felipe Balbi <balbi@...nel.org>,
        Balaji Prakash J <bjagadee@...eaurora.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Robert Hancock <robert.hancock@...ian.com>,
        Baruch Siach <baruch@...s.co.il>
Subject: Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

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@...o.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

Powered by blists - more mailing lists