Hi Felipe, On 11/9/2018 12:54 AM, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx> writes: >>> Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx> writes: >>>>> Thinh Nguyen <thinh.nguyen@xxxxxxxxxxxx> 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. > I had some misunderstanding with the purpose of GUCTL.REFCLKPER. After a discussion with the RTL engineers, it's not to control the reference clock. Setting this register field does not control the reference clock rate. The controller uses this value to perform internal timing calculations that are based on the reference clock. So, we will still need this property to set this value. I will resend the patch series with some changes and correct the commit messages with the proper definition of this feature. Thanks, Thinh
