linux-kernel - Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Jan 2022 12:56:04 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Sean Anderson <sean.anderson@...o.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.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


Hi,

Thinh Nguyen <Thinh.Nguyen@...opsys.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@...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.

that's also part of the coding style and probably documented somewhere

-- 
balbi