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. > I forgot to mention. DWC3 creates and use a platform device from PCI. BR, Thinh >> >>> 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. > >> >>> 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" > > 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. > > 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. > > BR, > Thinh > >> >> 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 >
