From: Sean Anderson <sean.anderson@seco.com>
To: Robert Hancock <robert.hancock@calian.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>,
"mounika.grace.akula@xilinx.com" <mounika.grace.akula@xilinx.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"Thinh.Nguyen@synopsys.com" <Thinh.Nguyen@synopsys.com>,
"michal.simek@xilinx.com" <michal.simek@xilinx.com>,
"balbi@kernel.org" <balbi@kernel.org>,
"manish.narani@xilinx.com" <manish.narani@xilinx.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration
Date: Fri, 14 Jan 2022 14:56:23 -0500 [thread overview]
Message-ID: <40b11f02-676f-a707-f27a-113c3ee3919e@seco.com> (raw)
In-Reply-To: <d416c324097a4feab4fa38d64a770d2a099d36c8.camel@calian.com>
On 1/14/22 2:22 PM, Robert Hancock wrote:
> On Fri, 2022-01-14 at 13:05 -0500, Sean Anderson wrote:
>> Hi Robert,
>>
>> On 1/13/22 11:42 PM, Robert Hancock wrote:
>> > Previously a device tree property was added to allow overriding the
>> > reference clock period parameter if the default value used was incorrect.
>> > However, there is another register field, GFLADJ_REFCLK_FLADJ, which
>> > reflects the fractional nanosecond portion of the reference clock
>> > period. Add a snps,ref-clock-fladj property to allow configuring this
>> > as well.
>> >
>> > On the Xilinx ZynqMP platform, the reference clock appears to always
>> > be 20 MHz, giving a clock period of 50 ns. However, the default value
>> > of GFLADJ_REFCLK_FLADJ was 1008 rather than 0 as it should have been,
>> > which prevented many USB devices from functioning properly. The
>> > psu_init code run by the Xilinx first-stage boot loader sets this
>> > value to 0, however when the controller is reset by the dwc3-xilinx
>> > layer, the incorrect default value is restored. This configuration
>> > property allows ensuring that the correct value is always used.
>> >
>> > Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
>> > ---
>> > drivers/usb/dwc3/core.c | 35 +++++++++++++++++++++++++++++++++++
>> > drivers/usb/dwc3/core.h | 5 +++++
>> > 2 files changed, 40 insertions(+)
>> >
>> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> > index f4c09951b517..ad224fb8088e 100644
>> > --- a/drivers/usb/dwc3/core.c
>> > +++ b/drivers/usb/dwc3/core.c
>> > @@ -359,6 +359,37 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> > }
>> >
>> >
>> > +/**
>> > + * dwc3_ref_clk_fladj - Reference clock period adjustment configuration
>> > + * @dwc: Pointer to our controller context structure
>> > + *
>> > + * GFLADJ_REFCLK_FLADJ should be set based on the fractional portion of
>> > the
>> > + * reference clock period, where the integer portion is set in
>> > GUCTL_REFCLKPER.
>> > + * Calculated as: ((125000/ref_clk_period_integer)-
>> > (125000/ref_clk_period)) * ref_clk_period
>> > + * where ref_clk_period_integer is the period specified in GUCTL_REFCLKPER
>> > and
>> > + * ref_clk_period is the period including fractional value.
>> > + * This value can be specified in the device tree if the default value is
>> > incorrect.
>> > + * Note that 0 is a valid value.
>> > + */
>> > +static void dwc3_ref_clk_fladj(struct dwc3 *dwc)
>> > +{
>> > + u32 reg;
>> > + u32 reg_new;
>> > +
>> > + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>> > + return;
>> > +
>> > + if (!dwc->ref_clk_fladj_set)
>> > + return;
>> > +
>> > + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>> > + reg_new = reg & ~DWC3_GFLADJ_REFCLK_FLADJ_MASK;
>> > + reg_new |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, dwc-
>> > >ref_clk_fladj);
>> > + if (reg_new != reg)
>> > + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg_new);
>> > +}
>> > +
>> > +
>> > /**
>> > * dwc3_free_one_event_buffer - Frees one event buffer
>> > * @dwc: Pointer to our controller context structure
>> > @@ -1033,6 +1064,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> >
>> > /* Adjust Reference Clock Period */
>> > dwc3_ref_clk_period(dwc);
>> > + dwc3_ref_clk_fladj(dwc);
>> >
>> > dwc3_set_incr_burst_type(dwc);
>> >
>> > @@ -1418,6 +1450,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>> > &dwc->fladj);
>> > device_property_read_u32(dev, "snps,ref-clock-period-ns",
>> > &dwc->ref_clk_per);
>> > + if (!device_property_read_u32(dev, "snps,ref-clock-fladj",
>> > + &dwc->ref_clk_fladj))
>> > + dwc->ref_clk_fladj_set = true;
>> >
>> > dwc->dis_metastability_quirk = device_property_read_bool(dev,
>> > "snps,dis_metastability_quirk");
>> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> > index e1cc3f7398fb..5011296786de 100644
>> > --- a/drivers/usb/dwc3/core.h
>> > +++ b/drivers/usb/dwc3/core.h
>> > @@ -388,6 +388,7 @@
>> > /* Global Frame Length Adjustment Register */
>> > #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
>> > #define DWC3_GFLADJ_30MHZ_MASK 0x3f
>> > +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK 0x3fff00
>> >
>> > /* Global User Control Register*/
>> > #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
>> > @@ -985,6 +986,8 @@ struct dwc3_scratchpad_array {
>> > * @regs_size: address space size
>> > * @fladj: frame length adjustment
>> > * @ref_clk_per: reference clock period configuration
>> > + * @ref_clk_fladj_set: whether ref_clk_fladj value is set/valid
>> > + * @ref_clk_fladj: reference clock period fractional adjustment
>> > * @irq_gadget: peripheral controller's IRQ number
>> > * @otg_irq: IRQ number for OTG IRQs
>> > * @current_otg_role: current role of operation while using the OTG block
>> > @@ -1166,6 +1169,8 @@ struct dwc3 {
>> >
>> > u32 fladj;
>> > u32 ref_clk_per;
>> > + bool ref_clk_fladj_set;
>> > + u32 ref_clk_fladj;
>> > u32 irq_gadget;
>> > u32 otg_irq;
>> > u32 current_otg_role;
>> >
>>
>> Doesn't this property already exist as snps,quirk-frame-length-adjustment?
>
> No, it's actually a different setting, though it's a bit confusing (kind of
> reflecting that the actual register settings are a bit confusing):
>
> snps,ref-clock-period-ns and snps,ref-clock-fladj specify the reference clock
> period (the whole nanoseconds and the fractional portion respectively).
>
> snps,quirk-frame-length-adjustment is another setting which seems to reflect
> the frame length according to a 30 MHz clock, and which overrides another input
> value that's provided to the core in hardware. (That's my understanding anyway,
> just based on the register descriptions in the ZynqMP documentation. The
> Synopsys guys might have a better idea what this actually does.)
I think it does the same thing, but when GFLADJ_REFCLK_LPM_SEL=0. Its
description is
> This field indicates the value that is used for frame length
> adjustment instead of considering from the sideband input signal
> fladj_30mhz_reg
and the description for GFLADJ_REFCLK_FLADJ is
> This field indicates the frame length adjustment to be applied when
> SOF/ITP counter is running on the ref_clk. This register value is used
> to adjust the ITP interval when GCTL[SOFITPSYNC] is set to '1'; SOF
> and ITP interval when GLADJ.GFLADJ_REFCLK_LPM_SEL is set to '1'.
Although it's possible that these are different aspects (interval vs
length?).
According to the xHCI standard section 5.2.4, "the default value
[of GFLADJ_30MHZ] is decimal 32 (20h), which gives a SOF cycle time of
60000." All in-tree bindings which set this property just set it to
0x20, so maybe we should just remove the binding and set it all the
time.
>>
>> ---
>>
>> I realize the cat is already out of the bag, but this seems like
>> something which could be better modeled with a frequency property, or by
>> using a clock. With these bindings, the board maintainer has to
>> determine the reference clock frequency and then manually calculate the
>> fractional adjustment. If the reference clock is ever changed (e.g. in
>> a new board revision), the maintainer must then update two properties.
>> However, Linux could calculate all this automatically.
>>
>> We already have a clock input for the reference with which we can
>> determine the frequency (according to bindings/usb/snps,dwc3.yaml,
>> though I cannot see where it is implemented in the driver). Even on
>> platforms without a reference clock (such as USB over PCIe [1]), one can
>> just add a fixed-rate clock to act as the reference.
>
> That probably would make some sense.. the complication is that at least looking
> at the ZynqMP setup for this USB core, in the device tree the top-level zynqmp-
> dwc3 "wrapper" driver (drivers/usb/dwc3/dwc3-xilinx.c) is the one that has the
> clocks mapped to it right now, not the actual snps,dwc3 device that this driver
> is operating with. Other dwc3 variants like exynos, imx8mp, qcom etc. seem to
> be set up similarly.
>
> I'm not actually sure why it was setup this way with a parent and child device
> node with separate drivers, rather than just having device-specific extensions
> in the dwc3 driver for implementations that need it. But I'm guessing there's
> probably enough device tree references to that setup that we're stuck with it
> now.
I believe the motivation is that several of these drivers have auxiliary
registers and rather than creating a second entry in `regs`, the
original authors created sub-nodes instead.
> Simplified example:
>
> usb0: usb@ff9d0000 {
> compatible = "xlnx,zynqmp-dwc3";
> clock-names
> = "bus_clk", "ref_clk";
> clocks = <&zynqmp_clk USB0_BUS_REF>, <&zynqmp_clk
> USB3_DUAL_REF>;
>
> dwc3_0: usb@fe200000 {
> compatible = "snps,dwc3";
> snps,quirk-frame-length-adjustment = <0x20>;
> snps,ref-clock-period-ns = <50>;
> snps,ref-clock-fladj = <0>;
> };
> };
>
>
> I'm pretty sure that the "ref_clk" clock on the zynqmp-dwc3 device is what
> snps,dwc3 calls the "ref" clock which these period settings are dealing with,
> and currently doesn't do anything with in its code if it's provided, other than
> enabling it. As you say, the driver could just pull out the rate of that clock
> and calculate the correct clock period values on its own.
I believe that's correct. On my system, ref_clk is set to usb3_dual_ref,
which is running at 20MHz.
> But given that properties need to be added to the device tree to support the
> current approach anyway, I guess the device tree should just add the ref clock
> into the child node as well..
I think that's a good idea. For existing bindings, we can create a
fixed-rate clock based on snps,ref-clock-period-ns (and mark that
property as deprecated).
--Sean
next prev parent reply other threads:[~2022-01-14 19:56 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-14 4:42 [PATCH v4 0/5] Xilinx ZynqMP USB fixes Robert Hancock
2022-01-14 4:42 ` [PATCH v4 1/5] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode Robert Hancock
2022-01-14 4:42 ` [PATCH v4 2/5] usb: dwc3: xilinx: Fix error handling when getting USB3 PHY Robert Hancock
2022-01-14 4:42 ` [PATCH v4 3/5] dt-bindings: usb: dwc3: add reference clock period fractional adjustment Robert Hancock
2022-01-14 4:42 ` [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration Robert Hancock
2022-01-14 18:05 ` Sean Anderson
2022-01-14 19:22 ` Robert Hancock
2022-01-14 19:56 ` Sean Anderson [this message]
2022-01-14 4:42 ` [PATCH v4 5/5] arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration Robert Hancock
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40b11f02-676f-a707-f27a-113c3ee3919e@seco.com \
--to=sean.anderson@seco.com \
--cc=Thinh.Nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
--cc=manish.narani@xilinx.com \
--cc=michal.simek@xilinx.com \
--cc=mounika.grace.akula@xilinx.com \
--cc=robert.hancock@calian.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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).