linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Robert Hancock <robert.hancock@calian.com>
To: "sean.anderson@seco.com" <sean.anderson@seco.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 19:22:50 +0000	[thread overview]
Message-ID: <d416c324097a4feab4fa38d64a770d2a099d36c8.camel@calian.com> (raw)
In-Reply-To: <cf9ec164-7d9f-8209-5d2a-8e8c26a7be7e@seco.com>

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 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.

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.

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..

> 
> --Sean
> 
> [1] 
> https://urldefense.com/v3/__https://lore.kernel.org/all/9f399bdf1ff752e31ab7497e3d5ce19bbb3ff247.1630389452.git.baruch@tkos.co.il/__;!!IOGos0k!ytRCRnO1vDXkVSV3Nz06dhYdT5kiZU75gcjcs0bPss2UQM4mSwij8Wglzdem3ctNH5g$
>  

  reply	other threads:[~2022-01-14 19:23 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 [this message]
2022-01-14 19:56       ` Sean Anderson
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=d416c324097a4feab4fa38d64a770d2a099d36c8.camel@calian.com \
    --to=robert.hancock@calian.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=robh+dt@kernel.org \
    --cc=sean.anderson@seco.com \
    --subject='Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration' \
    /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

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).