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

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

[Sean Anderson]

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/20220114044230.2677283-1-robert.hancock@calian.com/

Changes in v2:
- Document clock members
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a
- Add snps,ref-clock-frequency-hz property for ACPI

Sean Anderson (7):
  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: Program GFLADJ
  usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
  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                       | 112 +++++++++++++++---
 drivers/usb/dwc3/core.h                       |  17 ++-
 6 files changed, 120 insertions(+), 27 deletions(-)

-- 
2.25.1

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

[Sean Anderson]

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/20220114044230.2677283-1-robert.hancock@calian.com/

Changes in v2:
- Document clock members
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a
- Add snps,ref-clock-frequency-hz property for ACPI

Sean Anderson (7):
  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: Program GFLADJ
  usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
  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                       | 112 +++++++++++++++---
 drivers/usb/dwc3/core.h                       |  17 ++-
 6 files changed, 120 insertions(+), 27 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns

[Sean Anderson]

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 <sean.anderson@seco.com>
---

(no changes since v1)

 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

[PATCH v2 2/7] usb: dwc3: Get clocks individually

[Sean Anderson]

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 <sean.anderson@seco.com>
---

Changes in v2:
- Document clock members

 drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
 drivers/usb/dwc3/core.h | 10 ++++---
 2 files changed, 56 insertions(+), 16 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..45cfa7d9f27a 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -978,8 +978,9 @@ struct dwc3_scratchpad_array {
  * @eps: endpoint array
  * @gadget: device side representation of the peripheral controller
  * @gadget_driver: pointer to the gadget driver
- * @clks: array of clocks
- * @num_clks: number of clocks
+ * @bus_clk: clock for accessing the registers
+ * @ref_clk: reference clock
+ * @susp_clk: clock used when the SS phy is in low power (S3) state
  * @reset: reset control
  * @regs: base address for our registers
  * @regs_size: address space size
@@ -1134,8 +1135,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

[PATCH v2 3/7] usb: dwc3: Calculate REFCLKPER based on reference clock

[Sean Anderson]

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 <sean.anderson@seco.com>
---

(no changes since v1)

 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

[PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Sean Anderson]

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.

Additionally, calculate a value for 240MHZDECR. This configures a
simulated 240Mhz clock using a counter with one fractional bit (PLS1).

This register is programmed only for versions >= 2.50a, since this is
the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
adjustment quirk").

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a

 drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
 drivers/usb/dwc3/core.h |  3 +++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
 
 	if (dwc->ref_clk) {
 		rate = clk_get_rate(dwc->ref_clk);
@@ -357,6 +357,7 @@ 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;
 	}
@@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
 	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
 	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
 	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
+
+	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
+		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;
+
+	/*
+	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
+	 */
+	decr = 480000000 / rate;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
+	    &  ~DWC3_GFLADJ_240MHZDECR
+	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
+	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
+	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
+	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
+	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
 }
 
-
 /**
  * dwc3_free_one_event_buffer - Frees one event buffer
  * @dwc: Pointer to our controller context structure
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45cfa7d9f27a..eb9c1efced05 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -388,6 +388,9 @@
 /* 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)
+#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
+#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
 
 /* Global User Control Register*/
 #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
-- 
2.25.1

[PATCH v2 5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

[Sean Anderson]

This property allows setting the reference clock frequency properly for
ACPI-based systems. It is not documented under dt-bindings, since it is
not intended for use on DT-based systems. DT-based systems should use
the clocks property instead.

Frequency is preferred over period since it has greater precision when
used in calculations.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v2:
- New

 drivers/usb/dwc3/core.c | 6 ++++--
 drivers/usb/dwc3/core.h | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 883e119377f0..5f3dc5f6cbcb 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
 	u32 reg;
 	unsigned long decr, fladj, rate, period;
 
-	if (dwc->ref_clk) {
-		rate = clk_get_rate(dwc->ref_clk);
+	if (dwc->ref_clk || dwc->ref_clk_freq) {
+		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
 		if (!rate)
 			return;
 		period = NSEC_PER_SEC / rate;
@@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				 &dwc->fladj);
 	device_property_read_u32(dev, "snps,ref-clock-period-ns",
 				 &dwc->ref_clk_per);
+	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
+				 &dwc->ref_clk_freq);
 
 	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 eb9c1efced05..00a792459fec 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
  * @regs: base address for our registers
  * @regs_size: address space size
  * @fladj: frame length adjustment
- * @ref_clk_per: reference clock period configuration
+ * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
+ * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
  * @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
@@ -1171,6 +1172,7 @@ struct dwc3 {
 
 	u32			fladj;
 	u32			ref_clk_per;
+	u32			ref_clk_freq;
 	u32			irq_gadget;
 	u32			otg_irq;
 	u32			current_otg_role;
-- 
2.25.1

[PATCH v2 6/7] arm64: dts: zynqmp: Move USB clocks to dwc3 node

[Sean Anderson]

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 <sean.anderson@seco.com>
---

(no changes since v1)

 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: usb@ff9d0000 {
 			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: usb@fe200000 {
 				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: usb@ff9e0000 {
 			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: usb@fe300000 {
 				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

[PATCH v2 6/7] arm64: dts: zynqmp: Move USB clocks to dwc3 node

[Sean Anderson]

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 <sean.anderson@seco.com>
---

(no changes since v1)

 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: usb@ff9d0000 {
 			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: usb@fe200000 {
 				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: usb@ff9e0000 {
 			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: usb@fe300000 {
 				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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] arm64: dts: ipq6018: Use reference clock to set dwc3 period

[Sean Anderson]

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 <sean.anderson@seco.com>
---

(no changes since v1)

 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: usb@8A00000 {
 				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

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

[Baruch Siach]

Hi Sean,

On Tue, Jan 18 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.

Tested here on IPQ6010 based system. USB still works. But the with "ref"
clock at 24MHz, period is calculated as 0x29. Previous
snps,ref-clock-period-ns value used to be 0x32.

Is that expected?

Thanks,
baruch

>
> [1] https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>
> Changes in v2:
> - Document clock members
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
> - Add snps,ref-clock-frequency-hz property for ACPI
>
> Sean Anderson (7):
>   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: Program GFLADJ
>   usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>   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                       | 112 +++++++++++++++---
>  drivers/usb/dwc3/core.h                       |  17 ++-
>  6 files changed, 120 insertions(+), 27 deletions(-)


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Baruch Siach]

Hi Sean,

On Tue, Jan 18 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.

Tested here on IPQ6010 based system. USB still works. But the with "ref"
clock at 24MHz, period is calculated as 0x29. Previous
snps,ref-clock-period-ns value used to be 0x32.

Is that expected?

Thanks,
baruch

>
> [1] https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>
> Changes in v2:
> - Document clock members
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
> - Add snps,ref-clock-frequency-hz property for ACPI
>
> Sean Anderson (7):
>   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: Program GFLADJ
>   usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>   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                       | 112 +++++++++++++++---
>  drivers/usb/dwc3/core.h                       |  17 ++-
>  6 files changed, 120 insertions(+), 27 deletions(-)


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sean Anderson]

Hi Baruch,

On 1/19/22 1:14 PM, Baruch Siach wrote:
> Hi Sean,
>
> On Tue, Jan 18 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.
>
> Tested here on IPQ6010 based system. USB still works. But the with "ref"
> clock at 24MHz, period is calculated as 0x29. Previous
> snps,ref-clock-period-ns value used to be 0x32.
>
> Is that expected?

Yes. From the documentation for GFLADJ_REFCLK_240MHZ_DECR:

> Examples:
> If the ref_clk is 24 MHz then
> - GUCTL.REF_CLK_PERIOD = 41
> - GFLADJ.GFLADJ_REFCLK_240MHZ_DECR = 240/24 = 10

And 41 == 0x29.

--Sean

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

[Sean Anderson]

Hi Baruch,

On 1/19/22 1:14 PM, Baruch Siach wrote:
> Hi Sean,
>
> On Tue, Jan 18 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.
>
> Tested here on IPQ6010 based system. USB still works. But the with "ref"
> clock at 24MHz, period is calculated as 0x29. Previous
> snps,ref-clock-period-ns value used to be 0x32.
>
> Is that expected?

Yes. From the documentation for GFLADJ_REFCLK_240MHZ_DECR:

> Examples:
> If the ref_clk is 24 MHz then
> - GUCTL.REF_CLK_PERIOD = 41
> - GFLADJ.GFLADJ_REFCLK_240MHZ_DECR = 240/24 = 10

And 41 == 0x29.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kathiravan T]

On 2022-01-19 23:44, Baruch Siach wrote:
> Hi Sean,
> 
> On Tue, Jan 18 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.
> 
> Tested here on IPQ6010 based system. USB still works. But the with 
> "ref"
> clock at 24MHz, period is calculated as 0x29. Previous
> snps,ref-clock-period-ns value used to be 0x32.
> 
> Is that expected?
> 
> Thanks,
> baruch
> 


Hi Baruch,

Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly 
mentioned as 0x32, which was corrected recently.

Thanks,
Kathiravan T.

>> 
>> [1] 
>> https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>> 
>> Changes in v2:
>> - Document clock members
>> - Also program GFLADJ.240MHZDECR
>> - Don't program GFLADJ if the version is < 2.50a
>> - Add snps,ref-clock-frequency-hz property for ACPI
>> 
>> Sean Anderson (7):
>>   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: Program GFLADJ
>>   usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>   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                       | 112 
>> +++++++++++++++---
>>  drivers/usb/dwc3/core.h                       |  17 ++-
>>  6 files changed, 120 insertions(+), 27 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation

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

[Baruch Siach]

Hi Kathiravan,

On Thu, Jan 20 2022, Kathiravan T wrote:
> On 2022-01-19 23:44, Baruch Siach wrote:
>> Hi Sean,
>> On Tue, Jan 18 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.
>> Tested here on IPQ6010 based system. USB still works. But the with 
>> "ref"
>> clock at 24MHz, period is calculated as 0x29. Previous
>> snps,ref-clock-period-ns value used to be 0x32.
>> Is that expected?
>
> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
> as 0x32, which was corrected recently.

Thanks for the update. This needs fixing in upstream kernel. I'll send a
patch.

For some reason USB appears to work here with both values. Is it because
I only use USB2 signals? If this is the case them I can not actually
test this series on my system.

Thanks,
baruch

>>> [1] 
>>> https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>>> Changes in v2:
>>> - Document clock members
>>> - Also program GFLADJ.240MHZDECR
>>> - Don't program GFLADJ if the version is < 2.50a
>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>> Sean Anderson (7):
>>>   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: Program GFLADJ
>>>   usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>   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                       | 112 +++++++++++++++---
>>>  drivers/usb/dwc3/core.h                       |  17 ++-
>>>  6 files changed, 120 insertions(+), 27 deletions(-)


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

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

[Baruch Siach]

Hi Kathiravan,

On Thu, Jan 20 2022, Kathiravan T wrote:
> On 2022-01-19 23:44, Baruch Siach wrote:
>> Hi Sean,
>> On Tue, Jan 18 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.
>> Tested here on IPQ6010 based system. USB still works. But the with 
>> "ref"
>> clock at 24MHz, period is calculated as 0x29. Previous
>> snps,ref-clock-period-ns value used to be 0x32.
>> Is that expected?
>
> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
> as 0x32, which was corrected recently.

Thanks for the update. This needs fixing in upstream kernel. I'll send a
patch.

For some reason USB appears to work here with both values. Is it because
I only use USB2 signals? If this is the case them I can not actually
test this series on my system.

Thanks,
baruch

>>> [1] 
>>> https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>>> Changes in v2:
>>> - Document clock members
>>> - Also program GFLADJ.240MHZDECR
>>> - Don't program GFLADJ if the version is < 2.50a
>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>> Sean Anderson (7):
>>>   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: Program GFLADJ
>>>   usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>   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                       | 112 +++++++++++++++---
>>>  drivers/usb/dwc3/core.h                       |  17 ++-
>>>  6 files changed, 120 insertions(+), 27 deletions(-)


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] usb: dwc3: Get clocks individually

[Robert Hancock]

On Tue, 2022-01-18 at 19:24 -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.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Document clock members
> 
>  drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
>  drivers/usb/dwc3/core.h | 10 ++++---
>  2 files changed, 56 insertions(+), 16 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..45cfa7d9f27a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -978,8 +978,9 @@ struct dwc3_scratchpad_array {
>   * @eps: endpoint array
>   * @gadget: device side representation of the peripheral controller
>   * @gadget_driver: pointer to the gadget driver
> - * @clks: array of clocks
> - * @num_clks: number of clocks
> + * @bus_clk: clock for accessing the registers
> + * @ref_clk: reference clock
> + * @susp_clk: clock used when the SS phy is in low power (S3) state
>   * @reset: reset control
>   * @regs: base address for our registers
>   * @regs_size: address space size
> @@ -1134,8 +1135,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;
>  

Reviewed-by: Robert Hancock <robert.hancock@calian.com>

Re: [PATCH v2 3/7] usb: dwc3: Calculate REFCLKPER based on reference clock

[Robert Hancock]

On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> 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 <sean.anderson@seco.com>
> ---
> 
> (no changes since v1)
> 
>  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);
>  }
>  

Looks good here on a ZynqMP board.

Reviewed-by: Robert Hancock <robert.hancock@calian.com>
Tested-by: Robert Hancock <robert.hancock@calian.com>

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Robert Hancock]

On Tue, 2022-01-18 at 19:24 -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.
> 
> Additionally, calculate a value for 240MHZDECR. This configures a
> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
> 
> This register is programmed only for versions >= 2.50a, since this is
> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
> adjustment quirk").
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
> 
>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/dwc3/core.h |  3 +++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>  
>  	if (dwc->ref_clk) {
>  		rate = clk_get_rate(dwc->ref_clk);
> @@ -357,6 +357,7 @@ 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;
>  	}
> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> +
> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> +		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;
> +
> +	/*
> +	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
> +	 */
> +	decr = 480000000 / rate;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
> +	    &  ~DWC3_GFLADJ_240MHZDECR
> +	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>  }
>  
> -
>  /**
>   * dwc3_free_one_event_buffer - Frees one event buffer
>   * @dwc: Pointer to our controller context structure
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45cfa7d9f27a..eb9c1efced05 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -388,6 +388,9 @@
>  /* 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)
> +#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
> +#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
>  
>  /* Global User Control Register*/
>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000

Looks good here on a ZynqMP board. GFLADJ_REFCLK_FLADJ ends up being set to 1
now instead of 0, since the clock rate is actually 19999800 Hz rather than 20
MHz. But that should be correct, and USB still seems to work fine.

Reviewed-by: Robert Hancock <robert.hancock@calian.com>
Tested-by: Robert Hancock <robert.hancock@calian.com>

Re: [PATCH v2 6/7] arm64: dts: zynqmp: Move USB clocks to dwc3 node

[Robert Hancock]

On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> 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 <sean.anderson@seco.com>
> ---
> 
> (no changes since v1)
> 
>  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: usb@ff9d0000 {
>  			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: usb@fe200000 {
>  				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: usb@ff9e0000 {
>  			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: usb@fe300000 {
>  				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>;

Tested and working on ZynqMP.

Reviewed-by: Robert Hancock <robert.hancock@calian.com>
Tested-by: Robert Hancock <robert.hancock@calian.com>

Re: [PATCH v2 6/7] arm64: dts: zynqmp: Move USB clocks to dwc3 node

[Robert Hancock]

On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> 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 <sean.anderson@seco.com>
> ---
> 
> (no changes since v1)
> 
>  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: usb@ff9d0000 {
>  			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: usb@fe200000 {
>  				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: usb@ff9e0000 {
>  			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: usb@fe300000 {
>  				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>;

Tested and working on ZynqMP.

Reviewed-by: Robert Hancock <robert.hancock@calian.com>
Tested-by: Robert Hancock <robert.hancock@calian.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Kathiravan Thirumoorthy]

Hi Baruch,

On 1/20/2022 3:59 PM, Baruch Siach wrote:
> Hi Kathiravan,
>
> On Thu, Jan 20 2022, Kathiravan T wrote:
>> On 2022-01-19 23:44, Baruch Siach wrote:
>>> Hi Sean,
>>> On Tue, Jan 18 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.
>>> Tested here on IPQ6010 based system. USB still works. But the with
>>> "ref"
>>> clock at 24MHz, period is calculated as 0x29. Previous
>>> snps,ref-clock-period-ns value used to be 0x32.
>>> Is that expected?
>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
>> as 0x32, which was corrected recently.
> Thanks for the update. This needs fixing in upstream kernel. I'll send a
> patch.
>
> For some reason USB appears to work here with both values. Is it because
> I only use USB2 signals? If this is the case them I can not actually
> test this series on my system.

I could recollect we did see some issue on USB2.0 port as well, but it 
wasn't fatal one. Anyways it is better to test it.

Thanks,

Kathiravan T.

>
> Thanks,
> baruch
>
>>>> [1]
>>>> https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>>>> Changes in v2:
>>>> - Document clock members
>>>> - Also program GFLADJ.240MHZDECR
>>>> - Don't program GFLADJ if the version is < 2.50a
>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>> Sean Anderson (7):
>>>>    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: Program GFLADJ
>>>>    usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>>    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                       | 112 +++++++++++++++---
>>>>   drivers/usb/dwc3/core.h                       |  17 ++-
>>>>   6 files changed, 120 insertions(+), 27 deletions(-)
>

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

[Kathiravan Thirumoorthy]

Hi Baruch,

On 1/20/2022 3:59 PM, Baruch Siach wrote:
> Hi Kathiravan,
>
> On Thu, Jan 20 2022, Kathiravan T wrote:
>> On 2022-01-19 23:44, Baruch Siach wrote:
>>> Hi Sean,
>>> On Tue, Jan 18 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.
>>> Tested here on IPQ6010 based system. USB still works. But the with
>>> "ref"
>>> clock at 24MHz, period is calculated as 0x29. Previous
>>> snps,ref-clock-period-ns value used to be 0x32.
>>> Is that expected?
>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
>> as 0x32, which was corrected recently.
> Thanks for the update. This needs fixing in upstream kernel. I'll send a
> patch.
>
> For some reason USB appears to work here with both values. Is it because
> I only use USB2 signals? If this is the case them I can not actually
> test this series on my system.

I could recollect we did see some issue on USB2.0 port as well, but it 
wasn't fatal one. Anyways it is better to test it.

Thanks,

Kathiravan T.

>
> Thanks,
> baruch
>
>>>> [1]
>>>> https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/
>>>> Changes in v2:
>>>> - Document clock members
>>>> - Also program GFLADJ.240MHZDECR
>>>> - Don't program GFLADJ if the version is < 2.50a
>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>> Sean Anderson (7):
>>>>    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: Program GFLADJ
>>>>    usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>>    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                       | 112 +++++++++++++++---
>>>>   drivers/usb/dwc3/core.h                       |  17 ++-
>>>>   6 files changed, 120 insertions(+), 27 deletions(-)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

[Thinh Nguyen]

Sean Anderson wrote:
> This property allows setting the reference clock frequency properly for
> ACPI-based systems. It is not documented under dt-bindings, since it is
> not intended for use on DT-based systems. DT-based systems should use
> the clocks property instead.
> 
> Frequency is preferred over period since it has greater precision when
> used in calculations.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - New
> 
>  drivers/usb/dwc3/core.c | 6 ++++--
>  drivers/usb/dwc3/core.h | 4 +++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 883e119377f0..5f3dc5f6cbcb 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>  	u32 reg;
>  	unsigned long decr, fladj, rate, period;
>  
> -	if (dwc->ref_clk) {
> -		rate = clk_get_rate(dwc->ref_clk);
> +	if (dwc->ref_clk || dwc->ref_clk_freq) {
> +		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>  		if (!rate)
>  			return;
>  		period = NSEC_PER_SEC / rate;
> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				 &dwc->fladj);
>  	device_property_read_u32(dev, "snps,ref-clock-period-ns",
>  				 &dwc->ref_clk_per);
> +	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
> +				 &dwc->ref_clk_freq);

Please also document in dwc3 DT file whenever we add a new property.

Thanks,
Thinh

>  
>  	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 eb9c1efced05..00a792459fec 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
>   * @regs: base address for our registers
>   * @regs_size: address space size
>   * @fladj: frame length adjustment
> - * @ref_clk_per: reference clock period configuration
> + * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
> + * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
>   * @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
> @@ -1171,6 +1172,7 @@ struct dwc3 {
>  
>  	u32			fladj;
>  	u32			ref_clk_per;
> +	u32			ref_clk_freq;
>  	u32			irq_gadget;
>  	u32			otg_irq;
>  	u32			current_otg_role;

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Thinh Nguyen]

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.
> 
> Additionally, calculate a value for 240MHZDECR. This configures a
> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
> 
> This register is programmed only for versions >= 2.50a, since this is
> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
> adjustment quirk").
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v2:
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
> 
>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>  drivers/usb/dwc3/core.h |  3 +++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;

Minor style nit: Felipe prefers to keep the declaration on separate
lines, let's keep it consistent with the rest in this driver.

>  
>  	if (dwc->ref_clk) {
>  		rate = clk_get_rate(dwc->ref_clk);
> @@ -357,6 +357,7 @@ 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;
>  	}
> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> +
> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> +		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);

Can we rearrange the math so we don't have to operate on different data
type and deal with conversion/truncation?

> +	fladj -= 125000;
> +
> +	/*
> +	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
> +	 */
> +	decr = 480000000 / rate;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
> +	    &  ~DWC3_GFLADJ_240MHZDECR
> +	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);

Does this pass checkpatch?

> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>  }
>  
> -
>  /**
>   * dwc3_free_one_event_buffer - Frees one event buffer
>   * @dwc: Pointer to our controller context structure
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45cfa7d9f27a..eb9c1efced05 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -388,6 +388,9 @@
>  /* 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)
> +#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
> +#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
>  
>  /* Global User Control Register*/
>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
Thanks,
Thinh

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

[Thinh Nguyen]

Kathiravan Thirumoorthy wrote:
> Hi Baruch,
> 
> On 1/20/2022 3:59 PM, Baruch Siach wrote:
>> Hi Kathiravan,
>>
>> On Thu, Jan 20 2022, Kathiravan T wrote:
>>> On 2022-01-19 23:44, Baruch Siach wrote:
>>>> Hi Sean,
>>>> On Tue, Jan 18 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.
>>>> Tested here on IPQ6010 based system. USB still works. But the with
>>>> "ref"
>>>> clock at 24MHz, period is calculated as 0x29. Previous
>>>> snps,ref-clock-period-ns value used to be 0x32.
>>>> Is that expected?
>>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly
>>> mentioned
>>> as 0x32, which was corrected recently.
>> Thanks for the update. This needs fixing in upstream kernel. I'll send a
>> patch.
>>
>> For some reason USB appears to work here with both values. Is it because
>> I only use USB2 signals? If this is the case them I can not actually
>> test this series on my system.

The controller uses the GUCTL.REFCLKPER under specific conditions and
settings, and the conditions are different between host mode and device
mode. For example, if you're running as device-mode and not operating in
low power, and or not using periodic endpoints, you're not probably
testing this.

BR,
Thinh


> 
> I could recollect we did see some issue on USB2.0 port as well, but it
> wasn't fatal one. Anyways it is better to test it.
> 
> Thanks,
> 
> Kathiravan T.
> 
>>
>> Thanks,
>> baruch
>>
>>>>> [1]
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!MCqqTGYTR42-4_LUHhrSJjkUOkDZKb795I8lB3PY4dB-kVCBnR9YKucgzrwhlj0tZZm5$
>>>>> Changes in v2:
>>>>> - Document clock members
>>>>> - Also program GFLADJ.240MHZDECR
>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>>> Sean Anderson (7):
>>>>>    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: Program GFLADJ
>>>>>    usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>>>    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                       | 112
>>>>> +++++++++++++++---
>>>>>   drivers/usb/dwc3/core.h                       |  17 ++-
>>>>>   6 files changed, 120 insertions(+), 27 deletions(-)
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thinh Nguyen]

Kathiravan Thirumoorthy wrote:
> Hi Baruch,
> 
> On 1/20/2022 3:59 PM, Baruch Siach wrote:
>> Hi Kathiravan,
>>
>> On Thu, Jan 20 2022, Kathiravan T wrote:
>>> On 2022-01-19 23:44, Baruch Siach wrote:
>>>> Hi Sean,
>>>> On Tue, Jan 18 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.
>>>> Tested here on IPQ6010 based system. USB still works. But the with
>>>> "ref"
>>>> clock at 24MHz, period is calculated as 0x29. Previous
>>>> snps,ref-clock-period-ns value used to be 0x32.
>>>> Is that expected?
>>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly
>>> mentioned
>>> as 0x32, which was corrected recently.
>> Thanks for the update. This needs fixing in upstream kernel. I'll send a
>> patch.
>>
>> For some reason USB appears to work here with both values. Is it because
>> I only use USB2 signals? If this is the case them I can not actually
>> test this series on my system.

The controller uses the GUCTL.REFCLKPER under specific conditions and
settings, and the conditions are different between host mode and device
mode. For example, if you're running as device-mode and not operating in
low power, and or not using periodic endpoints, you're not probably
testing this.

BR,
Thinh


> 
> I could recollect we did see some issue on USB2.0 port as well, but it
> wasn't fatal one. Anyways it is better to test it.
> 
> Thanks,
> 
> Kathiravan T.
> 
>>
>> Thanks,
>> baruch
>>
>>>>> [1]
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!MCqqTGYTR42-4_LUHhrSJjkUOkDZKb795I8lB3PY4dB-kVCBnR9YKucgzrwhlj0tZZm5$
>>>>> Changes in v2:
>>>>> - Document clock members
>>>>> - Also program GFLADJ.240MHZDECR
>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>>> Sean Anderson (7):
>>>>>    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: Program GFLADJ
>>>>>    usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>>>    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                       | 112
>>>>> +++++++++++++++---
>>>>>   drivers/usb/dwc3/core.h                       |  17 ++-
>>>>>   6 files changed, 120 insertions(+), 27 deletions(-)
>>

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Sean Anderson]

On 1/24/22 5:46 PM, Thinh Nguyen wrote:
> 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.
>> 
>> Additionally, calculate a value for 240MHZDECR. This configures a
>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>> 
>> This register is programmed only for versions >= 2.50a, since this is
>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>> adjustment quirk").
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v2:
>> - Also program GFLADJ.240MHZDECR
>> - Don't program GFLADJ if the version is < 2.50a
>> 
>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>  drivers/usb/dwc3/core.h |  3 +++
>>  2 files changed, 38 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
> 
> Minor style nit: Felipe prefers to keep the declaration on separate
> lines, let's keep it consistent with the rest in this driver.

So 

unsigned int decr;
unsigned int fladj;
unsigned int rate;
unsigned int period;

?

Frankly that seems rather verbose.

>>  
>>  	if (dwc->ref_clk) {
>>  		rate = clk_get_rate(dwc->ref_clk);
>> @@ -357,6 +357,7 @@ 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;
>>  	}
>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>> +
>> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>> +		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);
> 
> Can we rearrange the math so we don't have to operate on different data
> type and deal with conversion/truncation?

I don't understand what data types you are referring to.

The truncation above (in the calculaion for rate/period) is intentional,
so we can determine the error introduced by representing period using
only ns.

>> +	fladj -= 125000;
>> +
>> +	/*
>> +	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>> +	 */
>> +	decr = 480000000 / rate;
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>> +	    &  ~DWC3_GFLADJ_240MHZDECR
>> +	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
>> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
> 
> Does this pass checkpatch?

Yes.

--Sean

>> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>>  }
>>  
>> -
>>  /**
>>   * dwc3_free_one_event_buffer - Frees one event buffer
>>   * @dwc: Pointer to our controller context structure
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 45cfa7d9f27a..eb9c1efced05 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -388,6 +388,9 @@
>>  /* 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)
>> +#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
>> +#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
>>  
>>  /* Global User Control Register*/
>>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
> Thanks,
> Thinh
> 

Re: [PATCH v2 5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

[Sean Anderson]

On 1/24/22 5:44 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>> This property allows setting the reference clock frequency properly for
>> ACPI-based systems. It is not documented under dt-bindings, since it is
>> not intended for use on DT-based systems. DT-based systems should use
>> the clocks property instead.
>> 
>> Frequency is preferred over period since it has greater precision when
>> used in calculations.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> 
>> Changes in v2:
>> - New
>> 
>>  drivers/usb/dwc3/core.c | 6 ++++--
>>  drivers/usb/dwc3/core.h | 4 +++-
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 883e119377f0..5f3dc5f6cbcb 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>  	u32 reg;
>>  	unsigned long decr, fladj, rate, period;
>>  
>> -	if (dwc->ref_clk) {
>> -		rate = clk_get_rate(dwc->ref_clk);
>> +	if (dwc->ref_clk || dwc->ref_clk_freq) {
>> +		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>>  		if (!rate)
>>  			return;
>>  		period = NSEC_PER_SEC / rate;
>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  				 &dwc->fladj);
>>  	device_property_read_u32(dev, "snps,ref-clock-period-ns",
>>  				 &dwc->ref_clk_per);
>> +	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>> +				 &dwc->ref_clk_freq);
> 
> Please also document in dwc3 DT file whenever we add a new property.

This is intentionally undocumented, as noted in the commit message. 
Rob Herring has said that dt-bindings should only document properties
intended for device-tree.

--Sean

Re: [PATCH v2 5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

[Sean Anderson]

On 1/24/22 6:07 PM, Sean Anderson wrote:
> On 1/24/22 5:44 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>> This property allows setting the reference clock frequency properly for
>>> ACPI-based systems. It is not documented under dt-bindings, since it is
>>> not intended for use on DT-based systems. DT-based systems should use
>>> the clocks property instead.
>>> 
>>> Frequency is preferred over period since it has greater precision when
>>> used in calculations.
>>> 
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>> 
>>> Changes in v2:
>>> - New
>>> 
>>>  drivers/usb/dwc3/core.c | 6 ++++--
>>>  drivers/usb/dwc3/core.h | 4 +++-
>>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 883e119377f0..5f3dc5f6cbcb 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>  	u32 reg;
>>>  	unsigned long decr, fladj, rate, period;
>>>  
>>> -	if (dwc->ref_clk) {
>>> -		rate = clk_get_rate(dwc->ref_clk);
>>> +	if (dwc->ref_clk || dwc->ref_clk_freq) {
>>> +		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>>>  		if (!rate)
>>>  			return;
>>>  		period = NSEC_PER_SEC / rate;
>>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>  				 &dwc->fladj);
>>>  	device_property_read_u32(dev, "snps,ref-clock-period-ns",
>>>  				 &dwc->ref_clk_per);
>>> +	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>>> +				 &dwc->ref_clk_freq);
>> 
>> Please also document in dwc3 DT file whenever we add a new property.
> 
> This is intentionally undocumented, as noted in the commit message. 
> Rob Herring has said that dt-bindings should only document properties
> intended for device-tree.

context: https://lore.kernel.org/all/20181219202734.GA31178@bogus/

This patch was later resubmitted as 24bc6e68efa0 ("serial: sc16is7xx: 
Respect clock-frequency property") without the dt-bindings documentation.

+CC Rob if he wants to comment on this specific situation.

--Sean

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Thinh Nguyen]

Sean Anderson wrote:
> 
> 
> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>> 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.
>>>
>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>
>>> This register is programmed only for versions >= 2.50a, since this is
>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>> adjustment quirk").
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Also program GFLADJ.240MHZDECR
>>> - Don't program GFLADJ if the version is < 2.50a
>>>
>>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>  drivers/usb/dwc3/core.h |  3 +++
>>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>
>> Minor style nit: Felipe prefers to keep the declaration on separate
>> lines, let's keep it consistent with the rest in this driver.
> 
> So 
> 
> unsigned int decr;
> unsigned int fladj;
> unsigned int rate;
> unsigned int period;
> 
> ?
> 
> Frankly that seems rather verbose.

A couple of the benefits of having it like this is to help with viewing
git-blame if we introduce new variables and help with backporting fix
patch a bit simpler. Mainly I'm just following Felipe's style and keep
it consistent in this driver, but I don't think it's a big deal.

> 
>>>  
>>>  	if (dwc->ref_clk) {
>>>  		rate = clk_get_rate(dwc->ref_clk);
>>> @@ -357,6 +357,7 @@ 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;
>>>  	}
>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>> +
>>> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>> +		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);
>>
>> Can we rearrange the math so we don't have to operate on different data
>> type and deal with conversion/truncation?
> 
> I don't understand what data types you are referring to.
> 
> The truncation above (in the calculaion for rate/period) is intentional,
> so we can determine the error introduced by representing period using
> only ns.

I was wondering if we rearrange the math so we don't need to cast and
use 64-bit here, but that may not be possible. Just computing/reviewing
in my head while accounting for truncation from 64-bit to 32-bit value
is taxing.

> 
>>> +	fladj -= 125000;
>>> +
>>> +	/*
>>> +	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>>> +	 */
>>> +	decr = 480000000 / rate;
>>> +
>>> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>>> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>>> +	    &  ~DWC3_GFLADJ_240MHZDECR
>>> +	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
>>> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
>>
>> Does this pass checkpatch?
> 
> Yes.
> 
> --Sean
> 
>>> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>>>  }
>>>  
>>> -
>>>  /**
>>>   * dwc3_free_one_event_buffer - Frees one event buffer
>>>   * @dwc: Pointer to our controller context structure
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 45cfa7d9f27a..eb9c1efced05 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -388,6 +388,9 @@
>>>  /* 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)
>>> +#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
>>> +#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
>>>  
>>>  /* Global User Control Register*/
>>>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000


Thanks,
Thinh

Re: [PATCH v2 5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

[Thinh Nguyen]

Sean Anderson wrote:
> 
> 
> On 1/24/22 6:07 PM, Sean Anderson wrote:
>> On 1/24/22 5:44 PM, Thinh Nguyen wrote:
>>> Sean Anderson wrote:
>>>> This property allows setting the reference clock frequency properly for
>>>> ACPI-based systems. It is not documented under dt-bindings, since it is
>>>> not intended for use on DT-based systems. DT-based systems should use
>>>> the clocks property instead.
>>>>
>>>> Frequency is preferred over period since it has greater precision when
>>>> used in calculations.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - New
>>>>
>>>>  drivers/usb/dwc3/core.c | 6 ++++--
>>>>  drivers/usb/dwc3/core.h | 4 +++-
>>>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 883e119377f0..5f3dc5f6cbcb 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>  	u32 reg;
>>>>  	unsigned long decr, fladj, rate, period;
>>>>  
>>>> -	if (dwc->ref_clk) {
>>>> -		rate = clk_get_rate(dwc->ref_clk);
>>>> +	if (dwc->ref_clk || dwc->ref_clk_freq) {
>>>> +		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>>>>  		if (!rate)
>>>>  			return;
>>>>  		period = NSEC_PER_SEC / rate;
>>>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>  				 &dwc->fladj);
>>>>  	device_property_read_u32(dev, "snps,ref-clock-period-ns",
>>>>  				 &dwc->ref_clk_per);
>>>> +	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>>>> +				 &dwc->ref_clk_freq);
>>>
>>> Please also document in dwc3 DT file whenever we add a new property.
>>
>> This is intentionally undocumented, as noted in the commit message. 
>> Rob Herring has said that dt-bindings should only document properties
>> intended for device-tree.
> 
> context: https://urldefense.com/v3/__https://lore.kernel.org/all/20181219202734.GA31178@bogus/__;!!A4F2R9G_pg!IFaZE73-RQzYnhLSPKuFGqreudBcyVi6T9lGG5byblzoC9i_diaCBe_soAyHPCupua_T$ 
> 
> This patch was later resubmitted as 24bc6e68efa0 ("serial: sc16is7xx: 
> Respect clock-frequency property") without the dt-bindings documentation.
> 
> +CC Rob if he wants to comment on this specific situation.
> 
> --Sean

If this is acceptable, that's great! It opens up more options to the PCI
glue drivers. Maybe we should also document inline for properties that
don't exist in the DT binding.

Thanks,
Thinh

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>> Sean Anderson wrote:
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>
>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>> lines, let's keep it consistent with the rest in this driver.
>> 
>> So 
>> 
>> unsigned int decr;
>> unsigned int fladj;
>> unsigned int rate;
>> unsigned int period;
>> 
>> ?
>> 
>> Frankly that seems rather verbose.
>
> A couple of the benefits of having it like this is to help with viewing
> git-blame if we introduce new variables and help with backporting fix

it also prevents single lines from being constantly modified just
because we're adding a new variable. In the diff, adding a new variable
should look like a new line being added, rather than modified.

> patch a bit simpler. Mainly I'm just following Felipe's style and keep

it's part of the kernel coding style, actually :-)

-- 
balbi

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Sean Anderson]

On 1/24/22 9:11 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>> 
>> 
>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>> 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.
>>>>
>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>
>>>> This register is programmed only for versions >= 2.50a, since this is
>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>> adjustment quirk").
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Also program GFLADJ.240MHZDECR
>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>
>>>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>  drivers/usb/dwc3/core.h |  3 +++
>>>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>
>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>> lines, let's keep it consistent with the rest in this driver.
>> 
>> So 
>> 
>> unsigned int decr;
>> unsigned int fladj;
>> unsigned int rate;
>> unsigned int period;
>> 
>> ?
>> 
>> Frankly that seems rather verbose.
> 
> A couple of the benefits of having it like this is to help with viewing
> git-blame if we introduce new variables and help with backporting fix
> patch a bit simpler. Mainly I'm just following Felipe's style and keep
> it consistent in this driver, but I don't think it's a big deal.

*shrug*

If it's the subsystem style I will rewrite it.

(btw is this documented anywhere for future contributors?)

>> 
>>>>  
>>>>  	if (dwc->ref_clk) {
>>>>  		rate = clk_get_rate(dwc->ref_clk);
>>>> @@ -357,6 +357,7 @@ 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;
>>>>  	}
>>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>>>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>>>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>>> +
>>>> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>>> +		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);
>>>
>>> Can we rearrange the math so we don't have to operate on different data
>>> type and deal with conversion/truncation?
>> 
>> I don't understand what data types you are referring to.
>> 
>> The truncation above (in the calculaion for rate/period) is intentional,
>> so we can determine the error introduced by representing period using
>> only ns.
> 
> I was wondering if we rearrange the math so we don't need to cast and
> use 64-bit here, but that may not be possible. Just computing/reviewing
> in my head while accounting for truncation from 64-bit to 32-bit value
> is taxing.

Well the primary issue is that log_2(125000ULL * NSEC_PER_SEC) ~= 49, so
we have to use 64-bit integers if we don't want to do any shifting of
the numerator. It might be possible to analyze the significant bits
through the calculation and determine that we can use less precision for
some of the intermediate calculations, but I haven't done that analysis.
IMO that sort of transformation would likely make the calculations more
difficult to understand, not less.

>> 
>>>> +	fladj -= 125000;
>>>> +
>>>> +	/*
>>>> +	 * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>>>> +	 */
>>>> +	decr = 480000000 / rate;
>>>> +
>>>> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>>>> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>>>> +	    &  ~DWC3_GFLADJ_240MHZDECR
>>>> +	    &  ~DWC3_GFLADJ_240MHZDECR_PLS1;
>>>> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>>>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>>>> +	    |  FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
>>>
>>> Does this pass checkpatch?
>> 
>> Yes.
>> 
>> --Sean
>> 
>>>> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>>>>  }
>>>>  
>>>> -
>>>>  /**
>>>>   * dwc3_free_one_event_buffer - Frees one event buffer
>>>>   * @dwc: Pointer to our controller context structure
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 45cfa7d9f27a..eb9c1efced05 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -388,6 +388,9 @@
>>>>  /* 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)
>>>> +#define DWC3_GFLADJ_240MHZDECR			GENMASK(30, 24)
>>>> +#define DWC3_GFLADJ_240MHZDECR_PLS1		BIT(31)
>>>>  
>>>>  /* Global User Control Register*/
>>>>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
> 
> 
> Thanks,
> Thinh
> 

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Thinh Nguyen]

Sean Anderson wrote:
> 
> 
> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>>
>>>
>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>> 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.
>>>>>
>>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>>
>>>>> This register is programmed only for versions >= 2.50a, since this is
>>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>>> adjustment quirk").
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Also program GFLADJ.240MHZDECR
>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>>
>>>>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>>  drivers/usb/dwc3/core.h |  3 +++
>>>>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>>
>>> So 
>>>
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>>
>>> ?
>>>
>>> Frankly that seems rather verbose.
>>
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>> it consistent in this driver, but I don't think it's a big deal.
> 
> *shrug*
> 
> If it's the subsystem style I will rewrite it.
> 

Felipe also prefers Reverse Christmas Tree style.

> (btw is this documented anywhere for future contributors?)
> 

I don't think it's documented, but Felipe Nak'ed patches that don't
follow this style before. I don't want this to be the main focus of the
conversation for this patch, but I don't want it to go unnoticed either.

Your concern is not alone regarding document (or the lacking of) for
different subsystem-specific styles.

(see https://lwn.net/Articles/758552/)

>>>
>>>>>  
>>>>>  	if (dwc->ref_clk) {
>>>>>  		rate = clk_get_rate(dwc->ref_clk);
>>>>> @@ -357,6 +357,7 @@ 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;
>>>>>  	}
>>>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>>  	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>>>>  	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>>>>  	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>>>> +
>>>>> +	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>>>> +		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);
>>>>
>>>> Can we rearrange the math so we don't have to operate on different data
>>>> type and deal with conversion/truncation?
>>>
>>> I don't understand what data types you are referring to.
>>>
>>> The truncation above (in the calculaion for rate/period) is intentional,
>>> so we can determine the error introduced by representing period using
>>> only ns.
>>
>> I was wondering if we rearrange the math so we don't need to cast and
>> use 64-bit here, but that may not be possible. Just computing/reviewing
>> in my head while accounting for truncation from 64-bit to 32-bit value
>> is taxing.
> 
> Well the primary issue is that log_2(125000ULL * NSEC_PER_SEC) ~= 49, so
> we have to use 64-bit integers if we don't want to do any shifting of
> the numerator. It might be possible to analyze the significant bits
> through the calculation and determine that we can use less precision for
> some of the intermediate calculations, but I haven't done that analysis.
> IMO that sort of transformation would likely make the calculations more
> difficult to understand, not less.

Fair enough.

Thanks,
Thinh

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Thinh Nguyen]

Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>> Sean Anderson wrote:
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>>
>>> So 
>>>
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>>
>>> ?
>>>
>>> Frankly that seems rather verbose.
>>
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
> 
> it also prevents single lines from being constantly modified just
> because we're adding a new variable. In the diff, adding a new variable
> should look like a new line being added, rather than modified.
> 

Right.

>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
> 
> it's part of the kernel coding style, actually :-)
> 

Glad to see you're here :)

BR,
Thinh

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Felipe Balbi]

Hi,

Sean Anderson <sean.anderson@seco.com> writes:

> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>> 
>>> So 
>>> 
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>> 
>>> ?
>>> 
>>> Frankly that seems rather verbose.
>> 
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>> it consistent in this driver, but I don't think it's a big deal.
>
> *shrug*
>
> If it's the subsystem style I will rewrite it.
>
> (btw is this documented anywhere for future contributors?)

https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting

"To this end, use just one data declaration per line (no commas for
multiple data declarations)"

-- 
balbi

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>>> 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.
>>>>>>
>>>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>>>
>>>>>> This register is programmed only for versions >= 2.50a, since this is
>>>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>>>> adjustment quirk").
>>>>>>
>>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Also program GFLADJ.240MHZDECR
>>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>>>
>>>>>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>>>  drivers/usb/dwc3/core.h |  3 +++
>>>>>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>>>
>>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>>> lines, let's keep it consistent with the rest in this driver.
>>>>
>>>> So 
>>>>
>>>> unsigned int decr;
>>>> unsigned int fladj;
>>>> unsigned int rate;
>>>> unsigned int period;
>>>>
>>>> ?
>>>>
>>>> Frankly that seems rather verbose.
>>>
>>> A couple of the benefits of having it like this is to help with viewing
>>> git-blame if we introduce new variables and help with backporting fix
>>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>>> it consistent in this driver, but I don't think it's a big deal.
>> 
>> *shrug*
>> 
>> If it's the subsystem style I will rewrite it.
>> 
>
> Felipe also prefers Reverse Christmas Tree style.

that's also part of the coding style and probably documented somewhere

-- 
balbi

Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Sean Anderson]

On 1/26/22 5:53 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Sean Anderson <sean.anderson@seco.com> writes:
> 
>> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 5214daceda86..883e119377f0 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 decr, fladj, rate, period;
>>>>>
>>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>>> lines, let's keep it consistent with the rest in this driver.
>>>> 
>>>> So 
>>>> 
>>>> unsigned int decr;
>>>> unsigned int fladj;
>>>> unsigned int rate;
>>>> unsigned int period;
>>>> 
>>>> ?
>>>> 
>>>> Frankly that seems rather verbose.
>>> 
>>> A couple of the benefits of having it like this is to help with viewing
>>> git-blame if we introduce new variables and help with backporting fix
>>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>>> it consistent in this driver, but I don't think it's a big deal.
>>
>> *shrug*
>>
>> If it's the subsystem style I will rewrite it.
>>
>> (btw is this documented anywhere for future contributors?)
> 
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> 
> "To this end, use just one data declaration per line (no commas for
> multiple data declarations)"
> 

This is just if you want to add comments.

--Sean