Re: [PATCH] usb: dwc3: reference clock configuration

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@codeaurora.org>
> >>>>>
> >>>>> 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@codeaurora.org>
> >>>>> [ baruch: mention tested hardware ]
> >>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >>>>> ---
> >>>>>  .../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