On Wed 07 Apr 21:50 CDT 2021, 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@xxxxxxxxxxxxxx> > >>>>> > >>>>> 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@xxxxxxxxxxxxxx> > >>>>> [ baruch: mention tested hardware ] > >>>>> Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx> > >>>>> --- > >>>>> .../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. > So we have a DWC3 controller on a PCI bus, somehow described in DT, but the refclock is derived from something that's not described as a clock in the OS? > > > >> 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" > As long as there's a technical reason why this needs to be described, this would be a property. > 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. > I'm not familiar with the details of these adjustments, but from what you describe your suggestion sounds very reasonable to me. > 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. > For things that are generic yes, but otherwise the general guideline is that things should be human readable with standard units (whenever possible). Thank you, Bjorn
