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