* [PATCH v4 0/5] Xilinx ZynqMP USB fixes @ 2022-01-14 4:42 Robert Hancock 2022-01-14 4:42 ` [PATCH v4 1/5] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode Robert Hancock ` (4 more replies) 0 siblings, 5 replies; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock Some fixes related to the DWC3 USB driver and Xilinx ZynqMP DWC3 wrapper driver to allow ZynqMP USB to work properly when the hardware is configured in USB 2.0-only mode. Changes since v3: -fixed DT schema dt-doc-validate error Changes since v2: -additional kerneldoc fixes Changes since v1: -added DT binding documentation for new attribute -kerneldoc formatting and reworded comments Robert Hancock (5): usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode usb: dwc3: xilinx: Fix error handling when getting USB3 PHY dt-bindings: usb: dwc3: add reference clock period fractional adjustment usb: dwc3: add reference clock FLADJ configuration arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +++++++ arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +++ drivers/usb/dwc3/core.c | 35 +++++++++++++++++++ drivers/usb/dwc3/core.h | 5 +++ drivers/usb/dwc3/dwc3-xilinx.c | 17 +++++---- 5 files changed, 67 insertions(+), 7 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/5] usb: dwc3: xilinx: Fix PIPE clock selection for USB2.0 mode 2022-01-14 4:42 [PATCH v4 0/5] Xilinx ZynqMP USB fixes Robert Hancock @ 2022-01-14 4:42 ` Robert Hancock 2022-01-14 4:42 ` [PATCH v4 2/5] usb: dwc3: xilinx: Fix error handling when getting USB3 PHY Robert Hancock ` (3 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock It appears that the PIPE clock should not be selected when only USB 2.0 is being used in the design and no USB 3.0 reference clock is used. Fix to set the correct value depending on whether a USB3 PHY is present. Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/usb/dwc3/dwc3-xilinx.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c index 9cc3ad701a29..3bc035376394 100644 --- a/drivers/usb/dwc3/dwc3-xilinx.c +++ b/drivers/usb/dwc3/dwc3-xilinx.c @@ -167,8 +167,11 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data) /* Set PIPE Power Present signal in FPD Power Present Register*/ writel(FPD_POWER_PRSNT_OPTION, priv_data->regs + XLNX_USB_FPD_POWER_PRSNT); - /* Set the PIPE Clock Select bit in FPD PIPE Clock register */ - writel(PIPE_CLK_SELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK); + /* Set the PIPE Clock Select bit in FPD PIPE Clock register if a USB3 + * PHY is in use, deselect otherwise + */ + writel(usb3_phy ? PIPE_CLK_SELECT : PIPE_CLK_DESELECT, + priv_data->regs + XLNX_USB_FPD_PIPE_CLK); ret = reset_control_deassert(crst); if (ret < 0) { -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/5] usb: dwc3: xilinx: Fix error handling when getting USB3 PHY 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 ` Robert Hancock 2022-01-14 4:42 ` [PATCH v4 3/5] dt-bindings: usb: dwc3: add reference clock period fractional adjustment Robert Hancock ` (2 subsequent siblings) 4 siblings, 0 replies; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock The code that looked up the USB3 PHY was ignoring all errors other than EPROBE_DEFER in an attempt to handle the PHY not being present. Fix and simplify the code by using devm_phy_optional_get and dev_err_probe so that a missing PHY is not treated as an error and unexpected errors are handled properly. Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/usb/dwc3/dwc3-xilinx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c index 3bc035376394..3b16e7610009 100644 --- a/drivers/usb/dwc3/dwc3-xilinx.c +++ b/drivers/usb/dwc3/dwc3-xilinx.c @@ -102,12 +102,12 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data) int ret; u32 reg; - usb3_phy = devm_phy_get(dev, "usb3-phy"); - if (PTR_ERR(usb3_phy) == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; + usb3_phy = devm_phy_optional_get(dev, "usb3-phy"); + if (IS_ERR(usb3_phy)) { + ret = PTR_ERR(usb3_phy); + dev_err_probe(dev, ret, + "failed to get USB3 PHY\n"); goto err; - } else if (IS_ERR(usb3_phy)) { - usb3_phy = NULL; } crst = devm_reset_control_get_exclusive(dev, "usb_crst"); -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 3/5] dt-bindings: usb: dwc3: add reference clock period fractional adjustment 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 ` Robert Hancock 2022-01-14 4:42 ` [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration Robert Hancock 2022-01-14 4:42 ` [PATCH v4 5/5] arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration Robert Hancock 4 siblings, 0 replies; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock Document the new snps,ref-clock-fladj property which can be used to set the fractional portion of the reference clock period. Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index d29ffcd27472..5872a4470b16 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -266,6 +266,19 @@ properties: minimum: 1 maximum: 0x3ff + snps,ref-clock-fladj: + description: + Value for GFLADJ_REFCLK_FLADJ field of GFLADJ register for the + fractional portion of the reference clock period in nanoseconds, + when the hardware set default does not match the actual + clock. Calculated via + ((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. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 124999 + snps,rx-thr-num-pkt-prd: description: Periodic ESS RX packet threshold count (host mode only). Set this and -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration 2022-01-14 4:42 [PATCH v4 0/5] Xilinx ZynqMP USB fixes Robert Hancock ` (2 preceding siblings ...) 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 ` Robert Hancock 2022-01-14 18:05 ` Sean Anderson 2022-01-14 4:42 ` [PATCH v4 5/5] arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration Robert Hancock 4 siblings, 1 reply; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock 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; -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration 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 0 siblings, 1 reply; 9+ messages in thread From: Sean Anderson @ 2022-01-14 18:05 UTC (permalink / raw) To: Robert Hancock, linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani 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? --- 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. --Sean [1] https://lore.kernel.org/all/9f399bdf1ff752e31ab7497e3d5ce19bbb3ff247.1630389452.git.baruch@tkos.co.il/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration 2022-01-14 18:05 ` Sean Anderson @ 2022-01-14 19:22 ` Robert Hancock 2022-01-14 19:56 ` Sean Anderson 0 siblings, 1 reply; 9+ messages in thread From: Robert Hancock @ 2022-01-14 19:22 UTC (permalink / raw) To: sean.anderson, linux-usb Cc: robh+dt, mounika.grace.akula, devicetree, Thinh.Nguyen, michal.simek, balbi, manish.narani, gregkh 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$ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration 2022-01-14 19:22 ` Robert Hancock @ 2022-01-14 19:56 ` Sean Anderson 0 siblings, 0 replies; 9+ messages in thread From: Sean Anderson @ 2022-01-14 19:56 UTC (permalink / raw) To: Robert Hancock, linux-usb Cc: robh+dt, mounika.grace.akula, devicetree, Thinh.Nguyen, michal.simek, balbi, manish.narani, gregkh 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 5/5] arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration 2022-01-14 4:42 [PATCH v4 0/5] Xilinx ZynqMP USB fixes Robert Hancock ` (3 preceding siblings ...) 2022-01-14 4:42 ` [PATCH v4 4/5] usb: dwc3: add reference clock FLADJ configuration Robert Hancock @ 2022-01-14 4:42 ` Robert Hancock 4 siblings, 0 replies; 9+ messages in thread From: Robert Hancock @ 2022-01-14 4:42 UTC (permalink / raw) To: linux-usb Cc: Thinh.Nguyen, robh+dt, devicetree, michal.simek, balbi, gregkh, mounika.grace.akula, manish.narani, Robert Hancock Set the reference clock period and FLADJ fields in the DWC3 USB driver to 50ns with no fractional portion to match the ZynqMP configuration. Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi index 74e66443e4ce..2f531707d5d4 100644 --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi @@ -828,6 +828,8 @@ dwc3_0: usb@fe200000 { #stream-id-cells = <1>; iommus = <&smmu 0x860>; snps,quirk-frame-length-adjustment = <0x20>; + snps,ref-clock-period-ns = <50>; + snps,ref-clock-fladj = <0>; /* dma-coherent; */ }; }; @@ -855,6 +857,8 @@ dwc3_1: usb@fe300000 { #stream-id-cells = <1>; iommus = <&smmu 0x861>; snps,quirk-frame-length-adjustment = <0x20>; + snps,ref-clock-period-ns = <50>; + snps,ref-clock-fladj = <0>; /* dma-coherent; */ }; }; -- 2.31.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-14 19:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2022-01-14 4:42 ` [PATCH v4 5/5] arm64: dts: zynqmp: Add DWC3 USB reference clock period configuration Robert Hancock
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).