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

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

Jack Pham wrote:
> Hi Kathiravan, Baruch,
> 
> On Thu, Feb 25, 2021 at 10:17:49PM +0530, Kathiravan T wrote:
>> On 2021-02-15 22:28, Kathiravan T wrote:
>>> On 2021-02-10 11:40, Baruch Siach wrote:
>>>> Hi Bjorn,
>>>>
>>>> Thanks for your review comments.
>>>>
>>>> On Mon, Feb 08 2021, Bjorn Andersson wrote:
>>>>> On Mon 08 Feb 00:00 CST 2021, 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.
>>>>>
>>>>> What are typical values for this property? What unit does it
>>>>> have? How
>>>>> does it actually relate to the frequency of the reference clock?
>>>>
>>>> Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
>>>> (24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this
>>>> value
>>>> appears to correlates with clock rate. I have no access to DWC3
>>>> documentation. I only tested IPQ6018 hardware.
>>>>
>>>
>>> It will be written as (0xA87F0 << 7) retaining the 0-7 LSB value.
>>> I could see, BIT(23) of GFLADJ register enables the functionality of
>>> running SOF/ITP counters based on the reference clock. Since this bit
>>> is set, we need to
>>> compute the other fields as well i.e., from 8th bit to 31st bit.
>>> Finally it is derived to
>>> 0xA87F0 for IPQ6018.
>>>
>>
>> Bjorn / All,
>>
>> Any comments on this? Please do suggest if this can be handled in a better
>> way.
>>
>>
>>>
>>>>>> + - 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.
>>>>>
>>>>> Can't we make the dwc3 reference this clock and use
>>>>> clk_get_rate() and
>>>>> then do this math in the driver?
>>>>
>>>> This is doable, I believe. Though current code does not identify
>>>> specific clocks, as far as I can see.
> 
> I agree it should be doable. Looks like prior to 0d3a97083e0c ("usb:
> dwc3: Rework clock initialization to be more flexible") the core did
> support specific clocks ("ref", "bus_early", "suspend"), but was
> changed to use a simpler devm_clk_bulk_get_all() call.
> 
>>> We can mention one more clock(ref) in the USB device node and do the
>>> math (NSEC_PER_SEC / clk_get_rate()) in dwc3 driver.
> 
> Yea, just need to make sure "ref" clk is specified in the DT node. Then
> in the driver you can just iterate through dwc->clks and try to find one
> with .id=="ref". If clk_get_rate() succeeds then you can use the value
> to calculate the GUCTL.REFCLKPER and GFLADJ register fields.
> 
> Or perhaps even use a lookup table, since according to the DWC3
> programming guide only 6 refclk frequencies (16, 17, 19.2, 20, 24, 39.7
> MHz) are supported so that might be simpler than a few integer divide
> operations that would otherwise be required.
> 
> Jack
> 

Hi,

Why not create use a DT property instead? This will complicate things
for PCI devices. The designer know what refclk frequencies it is, we
just need to inform the controller via this property in nanosecond. It's
more accurate and you don't have to any calculation or worry about
whether to match the "ref" clock, or worse create a dummy/fake refclk
for a PCI device just to inform the controller this.

BR,
Thinh