2022-01-15 02:11:30

by Sean Anderson

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

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.

[1] https://lore.kernel.org/linux-usb/[email protected]/


Sean Anderson (6):
dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
usb: dwc3: Get clocks individually
usb: dwc3: Calculate REFCLKPER based on reference clock
usb: dwc3: Handle fractional reference clocks
arm64: dts: zynqmp: Move USB clocks to dwc3 node
arm64: dts: ipq6018: Use reference clock to set dwc3 period

.../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
.../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
drivers/usb/dwc3/core.c | 98 ++++++++++++++++---
drivers/usb/dwc3/core.h | 6 +-
6 files changed, 98 insertions(+), 24 deletions(-)

--
2.25.1


2022-01-15 02:12:19

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 1/6] dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns

This property is redundant because we can determine the correct value for
REFCLKPER based on the "ref" clock. Deprecate it, and encourage users to
provide a clock instead. This also restricts the minimum and maximum to the
values documented in the register reference [1].

[1] https://www.xilinx.com/html_docs/registers/ug1087/usb3_xhci___guctl.html

Signed-off-by: Sean Anderson <[email protected]>
---

Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index d29ffcd27472..4f2b0913ad9f 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -263,8 +263,11 @@ properties:
Value for REFCLKPER field of GUCTL register for reference clock period in
nanoseconds, when the hardware set default does not match the actual
clock.
- minimum: 1
- maximum: 0x3ff
+
+ This binding is deprecated. Instead, provide an appropriate reference clock.
+ minimum: 8
+ maximum: 62
+ deprecated: true

snps,rx-thr-num-pkt-prd:
description:
--
2.25.1

2022-01-15 02:12:34

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 2/6] usb: dwc3: Get clocks individually

Instead of grabbing all clocks in bulk, grab them individually. This will
allow us to get the frequency or otherwise deal with discrete clocks. This
may break some platforms if they use a clock which doesn't use one of the
documented names.

Signed-off-by: Sean Anderson <[email protected]>
---

drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
drivers/usb/dwc3/core.h | 5 ++--
2 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f4c09951b517..699ab9abdc47 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -745,6 +745,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
return 0;
}

+static int dwc3_clk_enable(struct dwc3 *dwc)
+{
+ int ret;
+
+ ret = clk_prepare_enable(dwc->bus_clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(dwc->ref_clk);
+ if (ret)
+ goto disable_bus_clk;
+
+ ret = clk_prepare_enable(dwc->susp_clk);
+ if (ret)
+ goto disable_ref_clk;
+
+ return 0;
+
+disable_ref_clk:
+ clk_disable_unprepare(dwc->ref_clk);
+disable_bus_clk:
+ clk_disable_unprepare(dwc->bus_clk);
+ return ret;
+}
+
+static void dwc3_clk_disable(struct dwc3 *dwc)
+{
+ clk_disable_unprepare(dwc->susp_clk);
+ clk_disable_unprepare(dwc->ref_clk);
+ clk_disable_unprepare(dwc->bus_clk);
+}
+
static void dwc3_core_exit(struct dwc3 *dwc)
{
dwc3_event_buffers_cleanup(dwc);
@@ -758,7 +790,7 @@ static void dwc3_core_exit(struct dwc3 *dwc)
usb_phy_set_suspend(dwc->usb3_phy, 1);
phy_power_off(dwc->usb2_generic_phy);
phy_power_off(dwc->usb3_generic_phy);
- clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
+ dwc3_clk_disable(dwc);
reset_control_assert(dwc->reset);
}

@@ -1605,25 +1637,31 @@ static int dwc3_probe(struct platform_device *pdev)
return PTR_ERR(dwc->reset);

if (dev->of_node) {
- ret = devm_clk_bulk_get_all(dev, &dwc->clks);
- if (ret == -EPROBE_DEFER)
- return ret;
/*
* Clocks are optional, but new DT platforms should support all
* clocks as required by the DT-binding.
*/
- if (ret < 0)
- dwc->num_clks = 0;
- else
- dwc->num_clks = ret;
+ dwc->bus_clk = devm_clk_get_optional(dev, "bus_early");
+ if (IS_ERR(dwc->bus_clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->bus_clk),
+ "could not get bus clock\n");

+ dwc->ref_clk = devm_clk_get_optional(dev, "ref");
+ if (IS_ERR(dwc->ref_clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->ref_clk),
+ "could not get ref clock\n");
+
+ dwc->susp_clk = devm_clk_get_optional(dev, "suspend");
+ if (IS_ERR(dwc->susp_clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->susp_clk),
+ "could not get suspend clock\n");
}

ret = reset_control_deassert(dwc->reset);
if (ret)
return ret;

- ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
+ ret = dwc3_clk_enable(dwc);
if (ret)
goto assert_reset;

@@ -1711,7 +1749,7 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);

disable_clks:
- clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
+ dwc3_clk_disable(dwc);
assert_reset:
reset_control_assert(dwc->reset);

@@ -1755,7 +1793,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
if (ret)
return ret;

- ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
+ ret = dwc3_clk_enable(dwc);
if (ret)
goto assert_reset;

@@ -1766,7 +1804,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
return 0;

disable_clks:
- clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
+ dwc3_clk_disable(dwc);
assert_reset:
reset_control_assert(dwc->reset);

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e1cc3f7398fb..32dfcf3a83d5 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1134,8 +1134,9 @@ struct dwc3 {
struct usb_gadget *gadget;
struct usb_gadget_driver *gadget_driver;

- struct clk_bulk_data *clks;
- int num_clks;
+ struct clk *bus_clk;
+ struct clk *ref_clk;
+ struct clk *susp_clk;

struct reset_control *reset;

--
2.25.1

2022-01-15 02:13:37

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 3/6] usb: dwc3: Calculate REFCLKPER based on reference clock

Instead of using a special property to determine the reference clock
period, use the rate of the reference clock. When we have a legacy
snps,ref-clock-period-ns property and no reference clock, use it
instead. Fractional clocks are not currently supported, and will be
dealt with in the next commit.

Signed-off-by: Sean Anderson <[email protected]>
---

drivers/usb/dwc3/core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 699ab9abdc47..5214daceda86 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,13 +348,22 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
u32 reg;
+ unsigned long rate, period;

- if (dwc->ref_clk_per == 0)
+ if (dwc->ref_clk) {
+ rate = clk_get_rate(dwc->ref_clk);
+ if (!rate)
+ return;
+ period = NSEC_PER_SEC / rate;
+ } else if (dwc->ref_clk_per) {
+ period = dwc->ref_clk_per;
+ } else {
return;
+ }

reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
- reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, dwc->ref_clk_per);
+ reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
}

--
2.25.1

2022-01-15 02:13:59

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 4/6] usb: dwc3: Handle fractional reference clocks

GUCTL.REFCLKPER can only account for clock frequencies with integer
periods. To address this, program REFCLK_FLADJ with the relative error
caused by period truncation. The formula given in the register reference
has been rearranged to allow calculation based on rate (instead of
period), and to allow for fixed-point arithmetic.

Signed-off-by: Sean Anderson <[email protected]>
---

drivers/usb/dwc3/core.c | 25 +++++++++++++++++++++++--
drivers/usb/dwc3/core.h | 1 +
2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5214daceda86..48bb3e42cdd0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
u32 reg;
- unsigned long rate, period;
+ unsigned long fladj, rate, period;

if (dwc->ref_clk) {
rate = clk_get_rate(dwc->ref_clk);
@@ -357,16 +357,37 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
period = NSEC_PER_SEC / rate;
} else if (dwc->ref_clk_per) {
period = dwc->ref_clk_per;
+ rate = NSEC_PER_SEC / period;
} else {
return;
}

+ /*
+ * The calculation below is
+ *
+ * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
+ *
+ * but rearranged for fixed-point arithmetic.
+ *
+ * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
+ * nanoseconds of error caused by the truncation which happened during
+ * the division when calculating rate or period (whichever one was
+ * derived from the other). We first calculate the relative error, then
+ * scale it to units of 0.08%.
+ */
+ fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
+ fladj -= 125000;
+
reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
-}

+ reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+ reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK;
+ reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj);
+ dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
+}

/**
* dwc3_free_one_event_buffer - Frees one event buffer
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 32dfcf3a83d5..50c094af131d 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 GENMASK(21, 8)

/* Global User Control Register*/
#define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
--
2.25.1

2022-01-15 02:15:41

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 5/6] arm64: dts: zynqmp: Move USB clocks to dwc3 node

These clocks are not used by the dwc3-xilinx driver except to
enable/disable them. Move them to the dwc3 node so its driver can use
them to configure the reference clock period.

Signed-off-by: Sean Anderson <[email protected]>
---

arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 ++--
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
index 1e0b1bca7c94..8493dd7d5f1f 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
@@ -223,11 +223,11 @@ &uart1 {
clocks = <&zynqmp_clk UART1_REF>, <&zynqmp_clk LPD_LSBUS>;
};

-&usb0 {
+&dwc3_0 {
clocks = <&zynqmp_clk USB0_BUS_REF>, <&zynqmp_clk USB3_DUAL_REF>;
};

-&usb1 {
+&dwc3_1 {
clocks = <&zynqmp_clk USB1_BUS_REF>, <&zynqmp_clk USB3_DUAL_REF>;
};

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 74e66443e4ce..ba68fb8529ee 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -811,7 +811,6 @@ usb0: [email protected] {
status = "disabled";
compatible = "xlnx,zynqmp-dwc3";
reg = <0x0 0xff9d0000 0x0 0x100>;
- clock-names = "bus_clk", "ref_clk";
power-domains = <&zynqmp_firmware PD_USB_0>;
resets = <&zynqmp_reset ZYNQMP_RESET_USB0_CORERESET>,
<&zynqmp_reset ZYNQMP_RESET_USB0_HIBERRESET>,
@@ -825,6 +824,7 @@ dwc3_0: [email protected] {
interrupt-parent = <&gic>;
interrupt-names = "dwc_usb3", "otg";
interrupts = <0 65 4>, <0 69 4>;
+ clock-names = "bus_early", "ref";
#stream-id-cells = <1>;
iommus = <&smmu 0x860>;
snps,quirk-frame-length-adjustment = <0x20>;
@@ -838,7 +838,6 @@ usb1: [email protected] {
status = "disabled";
compatible = "xlnx,zynqmp-dwc3";
reg = <0x0 0xff9e0000 0x0 0x100>;
- clock-names = "bus_clk", "ref_clk";
power-domains = <&zynqmp_firmware PD_USB_1>;
resets = <&zynqmp_reset ZYNQMP_RESET_USB1_CORERESET>,
<&zynqmp_reset ZYNQMP_RESET_USB1_HIBERRESET>,
@@ -852,6 +851,7 @@ dwc3_1: [email protected] {
interrupt-parent = <&gic>;
interrupt-names = "dwc_usb3", "otg";
interrupts = <0 70 4>, <0 74 4>;
+ clock-names = "bus_early", "ref";
#stream-id-cells = <1>;
iommus = <&smmu 0x861>;
snps,quirk-frame-length-adjustment = <0x20>;
--
2.25.1

2022-01-15 02:16:19

by Sean Anderson

[permalink] [raw]
Subject: [PATCH 6/6] arm64: dts: ipq6018: Use reference clock to set dwc3 period

Instead of manually setting snps,ref-clock-period-ns, we can let the
driver calculate it automatically from the "ref" clock. I haven't
reviewed this board's schematics, so please let me know if this is the
wrong 24MHz clock to use.

Signed-off-by: Sean Anderson <[email protected]>
---

arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 66ec5615651d..a614b9f73e2c 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -743,12 +743,13 @@ dwc_0: [email protected] {
interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
phys = <&qusb_phy_0>, <&usb0_ssphy>;
phy-names = "usb2-phy", "usb3-phy";
+ clocks = <&xo>;
+ clock-names = "ref";
tx-fifo-resize;
snps,is-utmi-l1-suspend;
snps,hird-threshold = /bits/ 8 <0x0>;
snps,dis_u2_susphy_quirk;
snps,dis_u3_susphy_quirk;
- snps,ref-clock-period-ns = <0x32>;
dr_mode = "host";
};
};
--
2.25.1

2022-01-15 12:27:52

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH 2/6] usb: dwc3: Get clocks individually

On Fri, 2022-01-14 at 18:39 -0500, Sean Anderson wrote:
> Instead of grabbing all clocks in bulk, grab them individually. This will
> allow us to get the frequency or otherwise deal with discrete clocks. This
> may break some platforms if they use a clock which doesn't use one of the
> documented names.

Another approach would be to keep the bulk get and prepare_enable and just
search through the clocks in the bulk data to find the "ref" clock, i.e.
something like:

for (i = 0; i < dwc->num_clks; ++i)
if (!strcmp("ref", dwc->clks[i].id)) {
ref_clk_rate = clk_get_rate(dwc->clks[i].clk);
break;
}

That's probably simpler than all the extra complexity to get each of the clocks
individually and release them in the right order.

>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
> drivers/usb/dwc3/core.h | 5 ++--
> 2 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f4c09951b517..699ab9abdc47 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -745,6 +745,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> return 0;
> }
>
> +static int dwc3_clk_enable(struct dwc3 *dwc)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(dwc->bus_clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dwc->ref_clk);
> + if (ret)
> + goto disable_bus_clk;
> +
> + ret = clk_prepare_enable(dwc->susp_clk);
> + if (ret)
> + goto disable_ref_clk;
> +
> + return 0;
> +
> +disable_ref_clk:
> + clk_disable_unprepare(dwc->ref_clk);
> +disable_bus_clk:
> + clk_disable_unprepare(dwc->bus_clk);
> + return ret;
> +}
> +
> +static void dwc3_clk_disable(struct dwc3 *dwc)
> +{
> + clk_disable_unprepare(dwc->susp_clk);
> + clk_disable_unprepare(dwc->ref_clk);
> + clk_disable_unprepare(dwc->bus_clk);
> +}
> +
> static void dwc3_core_exit(struct dwc3 *dwc)
> {
> dwc3_event_buffers_cleanup(dwc);
> @@ -758,7 +790,7 @@ static void dwc3_core_exit(struct dwc3 *dwc)
> usb_phy_set_suspend(dwc->usb3_phy, 1);
> phy_power_off(dwc->usb2_generic_phy);
> phy_power_off(dwc->usb3_generic_phy);
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> reset_control_assert(dwc->reset);
> }
>
> @@ -1605,25 +1637,31 @@ static int dwc3_probe(struct platform_device *pdev)
> return PTR_ERR(dwc->reset);
>
> if (dev->of_node) {
> - ret = devm_clk_bulk_get_all(dev, &dwc->clks);
> - if (ret == -EPROBE_DEFER)
> - return ret;
> /*
> * Clocks are optional, but new DT platforms should support all
> * clocks as required by the DT-binding.
> */
> - if (ret < 0)
> - dwc->num_clks = 0;
> - else
> - dwc->num_clks = ret;
> + dwc->bus_clk = devm_clk_get_optional(dev, "bus_early");
> + if (IS_ERR(dwc->bus_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->bus_clk),
> + "could not get bus clock\n");
>
> + dwc->ref_clk = devm_clk_get_optional(dev, "ref");
> + if (IS_ERR(dwc->ref_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->ref_clk),
> + "could not get ref clock\n");
> +
> + dwc->susp_clk = devm_clk_get_optional(dev, "suspend");
> + if (IS_ERR(dwc->susp_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->susp_clk),
> + "could not get suspend clock\n");
> }
>
> ret = reset_control_deassert(dwc->reset);
> if (ret)
> return ret;
>
> - ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> + ret = dwc3_clk_enable(dwc);
> if (ret)
> goto assert_reset;
>
> @@ -1711,7 +1749,7 @@ static int dwc3_probe(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
>
> disable_clks:
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> assert_reset:
> reset_control_assert(dwc->reset);
>
> @@ -1755,7 +1793,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> if (ret)
> return ret;
>
> - ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> + ret = dwc3_clk_enable(dwc);
> if (ret)
> goto assert_reset;
>
> @@ -1766,7 +1804,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> return 0;
>
> disable_clks:
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> assert_reset:
> reset_control_assert(dwc->reset);
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e1cc3f7398fb..32dfcf3a83d5 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1134,8 +1134,9 @@ struct dwc3 {
> struct usb_gadget *gadget;
> struct usb_gadget_driver *gadget_driver;
>
> - struct clk_bulk_data *clks;
> - int num_clks;
> + struct clk *bus_clk;
> + struct clk *ref_clk;
> + struct clk *susp_clk;
>
> struct reset_control *reset;
>
--
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
http://www.calian.com

2022-01-15 12:31:58

by Robert Hancock

[permalink] [raw]
Subject: Re: [PATCH 4/6] usb: dwc3: Handle fractional reference clocks

On Fri, 2022-01-14 at 18:39 -0500, Sean Anderson wrote:
> GUCTL.REFCLKPER can only account for clock frequencies with integer
> periods. To address this, program REFCLK_FLADJ with the relative error
> caused by period truncation. The formula given in the register reference
> has been rearranged to allow calculation based on rate (instead of
> period), and to allow for fixed-point arithmetic.
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++++++--
> drivers/usb/dwc3/core.h | 1 +
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5214daceda86..48bb3e42cdd0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3
> *dwc)
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> u32 reg;
> - unsigned long rate, period;
> + unsigned long fladj, rate, period;
>
> if (dwc->ref_clk) {
> rate = clk_get_rate(dwc->ref_clk);
> @@ -357,16 +357,37 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> period = NSEC_PER_SEC / rate;
> } else if (dwc->ref_clk_per) {
> period = dwc->ref_clk_per;
> + rate = NSEC_PER_SEC / period;
> } else {
> return;
> }
>
> + /*
> + * The calculation below is
> + *
> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
> + *
> + * but rearranged for fixed-point arithmetic.
> + *
> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
> + * nanoseconds of error caused by the truncation which happened during
> + * the division when calculating rate or period (whichever one was
> + * derived from the other). We first calculate the relative error, then
> + * scale it to units of 0.08%.
> + */
> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
> + fladj -= 125000;
> +
> reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> -}
>
>

dwc3_frame_length_adjustment which also writes to the DWC3_GFLADJ register has
a conditional to skip it if DWC3_VER_IS_PRIOR(DWC3, 250A), not sure if it's
needed for this field or not?

> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK;
> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj);
> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> +}
>
> /**
> * dwc3_free_one_event_buffer - Frees one event buffer
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 32dfcf3a83d5..50c094af131d 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 GENMASK(21, 8)
>
> /* Global User Control Register*/
> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
--
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
http://www.calian.com

2022-01-17 17:09:40

by Baruch Siach

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

Hi Sean, Thinh,

On Fri, Jan 14 2022, Sean Anderson wrote:
> 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.
>
> [1] https://lore.kernel.org/linux-usb/[email protected]/

Thinh, you suggested the dedicated DT property for the reference clock:

https://lore.kernel.org/all/[email protected]/

Can you comment on this series?

Thanks,
baruch

> Sean Anderson (6):
> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
> usb: dwc3: Get clocks individually
> usb: dwc3: Calculate REFCLKPER based on reference clock
> usb: dwc3: Handle fractional reference clocks
> arm64: dts: zynqmp: Move USB clocks to dwc3 node
> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>
> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
> drivers/usb/dwc3/core.c | 98 ++++++++++++++++---
> drivers/usb/dwc3/core.h | 6 +-
> 6 files changed, 98 insertions(+), 24 deletions(-)


--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -

2022-01-18 02:24:27

by Jun Li

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

Sean Anderson <[email protected]> 于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.

Li Jun

>
> [1] https://lore.kernel.org/linux-usb/[email protected]/
>
>
> Sean Anderson (6):
> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
> usb: dwc3: Get clocks individually
> usb: dwc3: Calculate REFCLKPER based on reference clock
> usb: dwc3: Handle fractional reference clocks
> arm64: dts: zynqmp: Move USB clocks to dwc3 node
> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>
> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
> drivers/usb/dwc3/core.c | 98 ++++++++++++++++---
> drivers/usb/dwc3/core.h | 6 +-
> 6 files changed, 98 insertions(+), 24 deletions(-)
>
> --
> 2.25.1
>

2022-01-18 03:06:19

by Robert Hancock

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

On Mon, 2022-01-17 at 20:30 +0800, Jun Li wrote:
> Sean Anderson <[email protected]> 于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?

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.

> Li Jun
>
> > [1]
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/__;!!IOGos0k!387DmPelOIA5Z6ZSfzSF2spWPu3lARlrkdmIRGcPwlWDZGDQzdlEdEKBw1RWG8aRC38$
> >
> >
> >
> > Sean Anderson (6):
> > dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
> > usb: dwc3: Get clocks individually
> > usb: dwc3: Calculate REFCLKPER based on reference clock
> > usb: dwc3: Handle fractional reference clocks
> > arm64: dts: zynqmp: Move USB clocks to dwc3 node
> > arm64: dts: ipq6018: Use reference clock to set dwc3 period
> >
> > .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
> > arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
> > .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
> > arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
> > drivers/usb/dwc3/core.c | 98 ++++++++++++++++---
> > drivers/usb/dwc3/core.h | 6 +-
> > 6 files changed, 98 insertions(+), 24 deletions(-)
> >
> > --
> > 2.25.1
> >
--
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
http://www.calian.com