lkml.org 
[lkml]   [2022]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v2 4/7] usb: dwc3: Program GFLADJ
    Date
    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

    \
     
     \ /
      Last update: 2022-01-25 20:39    [W:3.326 / U:0.336 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site