Re: [PATCH 1/3] usb: dwc3: Add reference clock properties

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]


Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>>> Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>>> index 636630fb92d7..712b344c3a31 100644
>>>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>>> @@ -95,6 +95,24 @@ Optional properties:
>>>>>>>>>  			this and tx-thr-num-pkt-prd to a valid, non-zero value
>>>>>>>>>  			1-16 (DWC_usb31 programming guide section 1.2.3) to
>>>>>>>>>  			enable periodic ESS TX threshold.
>>>>>>>>> + - snps,refclk-period-ns: set to program the reference clock period. The valid
>>>>>>>>> +   			input periods are as follow:
>>>>>>>>> +			+-------------+-----------------+
>>>>>>>>> +			| Period (ns) | Freq (MHz)      |
>>>>>>>>> +			+-------------+-----------------+
>>>>>>>>> +			| 25          | 39.7/40         |
>>>>>>>>> +			| 41          | 24.4            |
>>>>>>>>> +			| 50          | 20              |
>>>>>>>>> +			| 52          | 19.2            |
>>>>>>>>> +			| 58          | 17.2            |
>>>>>>>>> +			| 62          | 16.1            |
>>>>>>>>> +			+-------------+-----------------+
>>>>>>>>> + - snps,enable-refclk-lpm: set to enable low power scheduling of isochronous
>>>>>>>>> +			transfers by running SOF/ITP counters using the
>>>>>>>>> +			reference clock. Only valid for DWC_usb31 peripheral
>>>>>>>>> +			controller v1.80a and higher. Both
>>>>>>>>> +			"snps,dis_u2_susphy_quirk" and
>>>>>>>>> +			"snps,dis_enblslpm_quirk" must not be set.
>>>>>>>> sounds like you should rely on clk API here. Then on driver call
>>>>>>>> clk_get_rate() to computer whatever you need to compute.
>>>>>>>>
>>>>>>> There's nothing to compute here. We can simply enable this feature with
>>>>>>> "snps, enable-refclk-lpm" and the controller will use the default refclk
>>>>>>> settings.
>>>>>> Right, right. What I'm saying, though, is that we have a clock API for
>>>>>> describing a clock. So why wouldn't we rely on that API for this? I
>>>>>> think both of these new properties can be replaced with standard clock
>>>>>> API properties:
>>>>>>
>>>>>> 	clocks = <&clk1>, ..., <&lpm_clk>
>>>>>>         clock-names = "clock1", ...., "lpm";
>>>>>>
>>>>>> Then dwc3 core could, simply, check if we have a clock named "lpm" and
>>>>>> if there is, use NSECS_PER_SEC / clk_get_rate() to get its period and
>>>>>> write it to the register that needs the information.
>>>>> There's no new clock here. We are using the ref_clk for SOF and ITP
>>>>> counter for this feature. Also, clocks are optional on non-DT platforms.
>>>>> To use the clock API, then we need to update the driver to allow some
>>>>> optional clock such as "ref" clock for non-DT platforms. Do you want to
>>>>> do it like this?
>>>> I can't think of a problem that would arise from that. Can you? Mark,
>>>> Rob, what do you think?
>>>>
>>> No problem. That can be done. This will remove the
>>> "snps,refclk-period-ns" property. But we should have
>>> "snps,enable-refclk-lpm" to enable this feature.
>> not really. Just check if you have a clock named lpm. If you do, then
>> you enable the feature.
>>
> But this clock name should be "ref".  The new name "lpm" would make it
> seem like it's a different clock.

now I understand. There's no special LPM clock, this is just the regular
old reference clock being used for LPM. I agree with you, only
refclk-period-ns will be replaced.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]