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

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

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.

> 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