From: Bjorn Andersson <firstname.lastname@example.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Kathiravan T <email@example.com>,
Baruch Siach <firstname.lastname@example.org>, Felipe Balbi <email@example.com>,
Andy Gross <firstname.lastname@example.org>,
Balaji Prakash J <email@example.com>
Subject: Re: [PATCH] usb: dwc3: reference clock configuration
Date: Wed, 7 Apr 2021 21:23:09 -0500 [thread overview]
Message-ID: <20210408022309.GL904837@yoga> (raw)
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 <firstname.lastname@example.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 <email@example.com>
> >>> [ baruch: mention tested hardware ]
> >>> Signed-off-by: Baruch Siach <firstname.lastname@example.org>
> >>> ---
> >>> .../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.
> 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.
next prev parent reply other threads:[~2021-04-08 2:23 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 6:00 [PATCH] usb: dwc3: reference clock configuration Baruch Siach
2021-02-08 6:56 ` Greg KH
2021-02-08 18:26 ` Bjorn Andersson
2021-02-10 6:10 ` Baruch Siach
2021-02-15 16:58 ` Kathiravan T
2021-02-25 16:47 ` Kathiravan T
2021-03-24 8:14 ` Jack Pham
2021-03-31 0:52 ` Thinh Nguyen
2021-03-31 1:17 ` Thinh Nguyen
2021-04-07 11:56 ` Kathiravan T
2021-04-08 1:53 ` Thinh Nguyen
2021-04-08 2:23 ` Bjorn Andersson [this message]
2021-04-08 2:50 ` Thinh Nguyen
2021-04-08 2:54 ` Thinh Nguyen
2021-04-08 3:15 ` Bjorn Andersson
2021-04-08 3:39 ` Thinh Nguyen
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).