Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Bjorn Andersson wrote:
> On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:
> 
>> Kathiravan T wrote:
>>> On 2021-03-31 06:47, Thinh Nguyen wrote:
>>>> Baruch Siach wrote:
>>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>
>>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>>> to control the behavior of controller with respect to SOF and ITP.
>>>>> The reset values of these registers are aligned for 19.2 MHz
>>>>> reference clock source. This change will add option to override
>>>>> these settings for reference clock other than 19.2 MHz
>>>>>
>>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>>>
>>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>> [ baruch: mention tested hardware ]
>>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>>> ---
>>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>>  3 files changed, 69 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
>>>>> of GFLADJ
>>>>>      register for post-silicon frame length adjustment when the
>>>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
>>>>> of GFLADJ
>>>>> +    register for reference clock other than 19.2 MHz is used.
>>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
>>>>> This field
>>>>> +    indicates in terms of nano seconds the period of ref_clk. To
>>>>> calculate the
>>>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>>>
>>>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
>>>> period to the controller here.
>>>>
>>>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
>>>> setting. The designer knows what period it should be and should properly
>>>> set it if the default is not correctly set.
>>>
>>> Thanks Thinh for your inputs. Can we have the DT property for both the
>>> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
>>> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
>>> In other words, are you fine with the
>>> approach followed here? If so, we can work on the nitpicks and send the V2.
>>>
>>> Please let us know your thoughts on this.
>>>
>>
>> Hi Kathiravan,
>>
>> Yes, IMO, using DT properties work just fine to inform the controller if
>> the default settings don't match the HW configuration.
> 
> I'm not against using a separate DT property if the information it
> provides can't be derived from what's already there.

That's the issue. That information is not available if dwc3 is using PCI
bus.

> 
>> As I mention in
>> the separate email thread, using clk_get_rate() doesn't make sense for
>> PCI devices.
>>
> 
> I'm sorry, can you help me understand why this relate to PCI?

It's because the clock's attributes are not exposed if we're using the
PCI bus. The driver cannot access this information unless the user
provides it in some other way.

> 
>> The snps,quirk-ref-clock-adjustment property updates multiple fields of
>> the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
>> to different properties for different fields for clarity. If the other
>> fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
>> according to the example of the programming guide, then do that
>> calculation in the driver as default.
> 
> It sounds to me that rather than saying "refclk is X MHz" you propose a
> set or properties in the line of "write X, Y, Z to these registers",
> which isn't what we typically put in DT.

Different fields of the register control different features and not just
the "refclk is X MHz".

GUCTL register field REFCLKPER is "refclk is X MHz"

GFLADJ register field GFLADJ_REFCLK_LPM_SEL enables a feature to use
refclk to track SOF/ITP counter

GFLADJ register field GFLADJ_REFCLK_FLADJ do adjustments to the frame
length when running on refclk

GFLADJ register field GFLADJ_REFCLK_240MHZ_DECR is another adjustment
for 240MHz

GFLADJ register field GFLADJ_REFCLK_240MHZDECR_PLS1 is another adjustment

My suggestion is to have 2 DT properties:
1) for GUCTL.REFCLKPER for "refclk is X MHz" but in term of period ns
2) for GFLADJ.GFLADJ_REFCLK_LPM_SEL to enable a specific feature of the
controller. The other fields of GFLADJ can be calculated as default
according to the programming guide.

Is it typical that we combine different features in a single DT
property? Which was what this orignal patch did for GFLADJ register with
the fields mentioned above.

BR,
Thinh

> 
> Regards,
> Bjorn
> 
>> However, I'd suggest to create a
>> separate property (maybe "snps,use-refclk-for-sof-itp"?) to select
>> GFLADJ.GFLADJ_REFCLK_LPM_SEL or GCTL.SOFITPSYNC depending whether the
>> controller is operating as host or device mode.
>> Note that this feature
>> is only available for DWC_usb32 and DWC_usb31 v1.80a or higher. I need
>> to double check for DWC_usb3 IP, but I believe it's v3.30a or higher.
>>
>> Btw, we don't need to mention 19.2 MHz since it's the specific default
>> configuration of your setup. Other setups may not be the same.
>>
>> BR,
>> Thinh