Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration

[Sean Anderson <sean.anderson@seco.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