Re: [PATCH 0/6] usb: dwc3: Calculate REFCLKPER et. al. from reference clock

[Sean Anderson <sean.anderson@seco.com>]

On 1/17/22 6:49 PM, Robert Hancock wrote:
> On Mon, 2022-01-17 at 20:30 +0800, Jun Li wrote:
>> Sean Anderson <sean.anderson@seco.com> 于2022年1月15日周六 10:11写道：
>> > This is a rework of patches 3-5 of [1]. It attempts to correctly program
>> > REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>> > we no longer need a special property duplicating this configuration,
>> > snps,ref-clock-period-ns is deprecated.
>> >
>> > Please test this! Patches 3/4 in this series have the effect of
>> > programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>> > the "ref" clock. I have build tested, but not much else.
>>
>> DWC3 databook states a *condition* for program those settings:
>>
>> This field must be programmed to a non-zero value only if
>> GFLADJ_REFCLK_LPM_SEL is set to '1' or GCTL.SOFITPSYNC is set to '1'.
>> The value is derived as follows:
>> FLADJ_REF_CLK_FLADJ=((125000/ref_clk_period_integer)-(125000/ref_clk_period))
>> * ref_clk_period where
>> ■ the ref_clk_period_integer is the integer value of the ref_clk
>> period got by truncating the decimal (fractional) value that is
>> programmed in the GUCTL.REF_CLK_PERIOD field.
>> ■ the ref_clk_period is the ref_clk period including the fractional value.
>>
>> So you may need a condition check, with that, only required users
>> are effected even with "ref" clock specified.
>>
>
> The Xilinx register documentation for this register in the DWC3 core (
> https://www.xilinx.com/html_docs/registers/ug1087/usb3_xhci___gfladj.html ) has
> the same description, but it is rather confusingly worded. I suspect what they
> really mean is that "this field only needs to be programmed if
> GFLADJ_REFCLK_LPM_SEL is set to '1' or GCTL.SOFITPSYNC is set to '1'", not
> "this field should only be programmed if GFLADJ_REFCLK_LPM_SEL is set to '1' or
> GCTL.SOFITPSYNC is set to '1'". I'm not sure if someone can confirm that
> interpretation is correct?

This is the interpretation I was using as well, since GUCTL.REFCLKPER
has similar documentation and it was always programmed before this
series.

> However, looking at that description a bit further, it looks like there are
> some other fields in that register which are dependent on the reference clock:
> GFLADJ_REFCLK_240MHZ_DECR (bits 30:24) and GFLADJ_REFCLK_240MHZDECR_PLS1 (bit
> 31). It looks like the Xilinx board I am using has those set properly, i.e. to
> 12 and 0 respectively (I'm guessing by hardware default, since I don't see
> anything in the FSBL psu_init code setting those), but it wouldn't hurt to
> ensure those fields are also set correctly.

I'll add those in v2.

--Sean