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

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

Thinh Nguyen wrote:
> 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.
> 

I forgot to mention. DWC3 creates and use a platform device from PCI.

BR,
Thinh

>>
>>> 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
>