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

[PATCH 0/6] 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/


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

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

-- 
2.25.1

[PATCH 1/6] 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>
---

 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 2/6] 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>
---

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

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

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

[Robert Hancock]

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

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

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

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

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

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

[Sean Anderson]

Hi Rob,

On 1/14/22 8:04 PM, Robert Hancock wrote:
> On Fri, 2022-01-14 at 18:39 -0500, Sean Anderson wrote:
>> Instead of grabbing all clocks in bulk, grab them individually. This will
>> allow us to get the frequency or otherwise deal with discrete clocks. This
>> may break some platforms if they use a clock which doesn't use one of the
>> documented names.
>
> Another approach would be to keep the bulk get and prepare_enable and just
> search through the clocks in the bulk data to find the "ref" clock, i.e.
> something like:
>
> 		for (i = 0; i < dwc->num_clks; ++i)
> 			if (!strcmp("ref", dwc->clks[i].id)) {
> 				ref_clk_rate = clk_get_rate(dwc->clks[i].clk);
> 				break;
> 			}
>
> That's probably simpler than all the extra complexity to get each of the clocks
> individually and release them in the right order.

Hm, maybe. But in the future, we may want to access other clocks as
well. E.g. perhaps PM might be enhanced by treating the bus/suspend
clock specially for some modes. I think grabbing the clocks up front
will make later patches easier to write.

--Sean

[PATCH 3/6] 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>
---

 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 4/6] usb: dwc3: Handle fractional reference clocks

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

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

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

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5214daceda86..48bb3e42cdd0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
 static void dwc3_ref_clk_period(struct dwc3 *dwc)
 {
 	u32 reg;
-	unsigned long rate, period;
+	unsigned long fladj, rate, period;
 
 	if (dwc->ref_clk) {
 		rate = clk_get_rate(dwc->ref_clk);
@@ -357,16 +357,37 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
 		period = NSEC_PER_SEC / rate;
 	} else if (dwc->ref_clk_per) {
 		period = dwc->ref_clk_per;
+		rate = NSEC_PER_SEC / period;
 	} else {
 		return;
 	}
 
+	/*
+	 * The calculation below is
+	 *
+	 * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
+	 *
+	 * but rearranged for fixed-point arithmetic.
+	 *
+	 * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
+	 * nanoseconds of error caused by the truncation which happened during
+	 * the division when calculating rate or period (whichever one was
+	 * derived from the other). We first calculate the relative error, then
+	 * scale it to units of 0.08%.
+	 */
+	fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
+	fladj -= 125000;
+
 	reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
 	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
 	reg |=  FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
 	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
-}
 
+	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK;
+	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj);
+	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
+}
 
 /**
  * dwc3_free_one_event_buffer - Frees one event buffer
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 32dfcf3a83d5..50c094af131d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -388,6 +388,7 @@
 /* Global Frame Length Adjustment Register */
 #define DWC3_GFLADJ_30MHZ_SDBND_SEL		BIT(7)
 #define DWC3_GFLADJ_30MHZ_MASK			0x3f
+#define DWC3_GFLADJ_REFCLK_FLADJ_MASK		GENMASK(21, 8)
 
 /* Global User Control Register*/
 #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
-- 
2.25.1

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

[Robert Hancock]

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

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

> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> +	reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK;
> +	reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj);
> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> +}
>  
>  /**
>   * dwc3_free_one_event_buffer - Frees one event buffer
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 32dfcf3a83d5..50c094af131d 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -388,6 +388,7 @@
>  /* Global Frame Length Adjustment Register */
>  #define DWC3_GFLADJ_30MHZ_SDBND_SEL		BIT(7)
>  #define DWC3_GFLADJ_30MHZ_MASK			0x3f
> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK		GENMASK(21, 8)
>  
>  /* Global User Control Register*/
>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

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

[Sean Anderson]

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

That's a good question...

The oldest version I found with it is 2.60a [1].

[1] https://www.mouser.com/pdfdocs/silver-celeron-datasheet-vol-2.pdf

--Sean

[PATCH 5/6] 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>
---

 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 6/6] 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>
---

 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 0/6] usb: dwc3: Calculate REFCLKPER et. al. from reference clock

[Baruch Siach]

Hi Sean, Thinh,

On Fri, Jan 14 2022, Sean Anderson wrote:
> This is a rework of patches 3-5 of [1]. It attempts to correctly program
> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
> we no longer need a special property duplicating this configuration,
> snps,ref-clock-period-ns is deprecated.
>
> Please test this! Patches 3/4 in this series have the effect of
> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
> the "ref" clock. I have build tested, but not much else.
>
> [1] https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/

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

  https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/

Can you comment on this series?

Thanks,
baruch

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


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

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

[Thinh Nguyen]

Hi Sean,

Baruch Siach wrote:
> Hi Sean, Thinh,
> 
> On Fri, Jan 14 2022, Sean Anderson wrote:
>> This is a rework of patches 3-5 of [1]. It attempts to correctly program
>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>> we no longer need a special property duplicating this configuration,
>> snps,ref-clock-period-ns is deprecated.
>>
>> Please test this! Patches 3/4 in this series have the effect of
>> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>> the "ref" clock. I have build tested, but not much else.
>>
>> [1] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$ 
> 
> Thinh, you suggested the dedicated DT property for the reference clock:
> 
>   https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$ 
> 
> Can you comment on this series?
> 

Unless there's a good way to pass this information for PCI devices, my
opinion hasn't changed. (Btw, I don't think creating a dummy clock
provider and its dummy ops is a good solution as seems to complicate and
bloat the PCI glue drivers).

Please help come up with a solution before deprecating the ref clock
property.

Thanks,
Thinh

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

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

[Sean Anderson]

Hi Thinh,

On 1/18/22 2:46 PM, Thinh Nguyen wrote:
> Hi Sean,
>
> Baruch Siach wrote:
>> Hi Sean, Thinh,
>>
>> On Fri, Jan 14 2022, Sean Anderson wrote:
>>> This is a rework of patches 3-5 of [1]. It attempts to correctly program
>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>>> we no longer need a special property duplicating this configuration,
>>> snps,ref-clock-period-ns is deprecated.
>>>
>>> Please test this! Patches 3/4 in this series have the effect of
>>> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>>> the "ref" clock. I have build tested, but not much else.
>>>
>>> [1] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
>>
>> Thinh, you suggested the dedicated DT property for the reference clock:
>>
>>   https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
>>
>> Can you comment on this series?
>>
>
> Unless there's a good way to pass this information for PCI devices, my
> opinion hasn't changed. (Btw, I don't think creating a dummy clock
> provider and its dummy ops is a good solution as seems to complicate and
> bloat the PCI glue drivers).

Can you explain your situation a bit more? I'm not sure how you can
access a device tree property but not add a fixed-rate clock.

--Sean

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

[Thinh Nguyen]

Sean Anderson wrote:
> Hi Thinh,
> 
> On 1/18/22 2:46 PM, Thinh Nguyen wrote:
>> Hi Sean,
>>
>> Baruch Siach wrote:
>>> Hi Sean, Thinh,
>>>
>>> On Fri, Jan 14 2022, Sean Anderson wrote:
>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>>>> program
>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>>>> Since
>>>> we no longer need a special property duplicating this configuration,
>>>> snps,ref-clock-period-ns is deprecated.
>>>>
>>>> Please test this! Patches 3/4 in this series have the effect of
>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>>>> configure
>>>> the "ref" clock. I have build tested, but not much else.
>>>>
>>>> [1]
>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
>>>>
>>>
>>> Thinh, you suggested the dedicated DT property for the reference clock:
>>>
>>>  
>>> https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
>>>
>>>
>>> Can you comment on this series?
>>>
>>
>> Unless there's a good way to pass this information for PCI devices, my
>> opinion hasn't changed. (Btw, I don't think creating a dummy clock
>> provider and its dummy ops is a good solution as seems to complicate and
>> bloat the PCI glue drivers).
> 
> Can you explain your situation a bit more? I'm not sure how you can
> access a device tree property but not add a fixed-rate clock.
> 
> --Sean

Currently for dwc3 pci devices, we have glue drivers that create a
platform_device with specific properties to pass to the dwc3 core
driver. Without a ref clock property, we would need another way to pass
this information to the core driver or another way for the dwc3 core
driver to check for specific pci device's properties and quirks.

BR,
Thinh

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

[Robert Hancock]

On Tue, 2022-01-18 at 20:00 +0000, Thinh Nguyen wrote:
> Sean Anderson wrote:
> > Hi Thinh,
> > 
> > On 1/18/22 2:46 PM, Thinh Nguyen wrote:
> > > Hi Sean,
> > > 
> > > Baruch Siach wrote:
> > > > Hi Sean, Thinh,
> > > > 
> > > > On Fri, Jan 14 2022, Sean Anderson wrote:
> > > > > This is a rework of patches 3-5 of [1]. It attempts to correctly
> > > > > program
> > > > > REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
> > > > > Since
> > > > > we no longer need a special property duplicating this configuration,
> > > > > snps,ref-clock-period-ns is deprecated.
> > > > > 
> > > > > Please test this! Patches 3/4 in this series have the effect of
> > > > > programming REFCLKPER and REFCLK_FLADJ on boards which already
> > > > > configure
> > > > > the "ref" clock. I have build tested, but not much else.
> > > > > 
> > > > > [1]
> > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
> > > > > 
> > > > 
> > > > Thinh, you suggested the dedicated DT property for the reference clock:
> > > > 
> > > >  
> > > > https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
> > > > 
> > > > 
> > > > Can you comment on this series?
> > > > 
> > > 
> > > Unless there's a good way to pass this information for PCI devices, my
> > > opinion hasn't changed. (Btw, I don't think creating a dummy clock
> > > provider and its dummy ops is a good solution as seems to complicate and
> > > bloat the PCI glue drivers).
> > 
> > Can you explain your situation a bit more? I'm not sure how you can
> > access a device tree property but not add a fixed-rate clock.
> > 
> > --Sean
> 
> Currently for dwc3 pci devices, we have glue drivers that create a
> platform_device with specific properties to pass to the dwc3 core
> driver. Without a ref clock property, we would need another way to pass
> this information to the core driver or another way for the dwc3 core
> driver to check for specific pci device's properties and quirks.

We've used the device tree to instantiate/configure devices inside of a PCI
device, though obviously that only works on DT-based platforms, and for
hardware that's part of the board itself, not an add-in card.

We've also used the MFD infrastructure to instantiate devices and device
properties inside a PCI device on x86, which can be used if the driver you are
instantiating uses the generic device property accessors and not the DT-
specific ones. That gets a bit dirty however - I don't think there's an easy
way to create properties that are references to other nodes, or more than a
single level deep heirarchy of nodes.

For a use case like you're describing, it sounds like it would be better to
abstract away some of the core DWC3 code from reading the settings from DT
directly, so that the PCI devices can instantiate it and set the configuration
however they want, without having to worry about creating fake properties for
the core to read. I think that pattern has been used with some other drivers
such as AHCI?

> 
> BR,
> Thinh
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

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

[Thinh Nguyen]

Robert Hancock wrote:
> On Tue, 2022-01-18 at 20:00 +0000, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>> Hi Thinh,
>>>
>>> On 1/18/22 2:46 PM, Thinh Nguyen wrote:
>>>> Hi Sean,
>>>>
>>>> Baruch Siach wrote:
>>>>> Hi Sean, Thinh,
>>>>>
>>>>> On Fri, Jan 14 2022, Sean Anderson wrote:
>>>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>>>>>> program
>>>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>>>>>> Since
>>>>>> we no longer need a special property duplicating this configuration,
>>>>>> snps,ref-clock-period-ns is deprecated.
>>>>>>
>>>>>> Please test this! Patches 3/4 in this series have the effect of
>>>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>>>>>> configure
>>>>>> the "ref" clock. I have build tested, but not much else.
>>>>>>
>>>>>> [1]
>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
>>>>>>
>>>>>
>>>>> Thinh, you suggested the dedicated DT property for the reference clock:
>>>>>
>>>>>  
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
>>>>>
>>>>>
>>>>> Can you comment on this series?
>>>>>
>>>>
>>>> Unless there's a good way to pass this information for PCI devices, my
>>>> opinion hasn't changed. (Btw, I don't think creating a dummy clock
>>>> provider and its dummy ops is a good solution as seems to complicate and
>>>> bloat the PCI glue drivers).
>>>
>>> Can you explain your situation a bit more? I'm not sure how you can
>>> access a device tree property but not add a fixed-rate clock.
>>>
>>> --Sean
>>
>> Currently for dwc3 pci devices, we have glue drivers that create a
>> platform_device with specific properties to pass to the dwc3 core
>> driver. Without a ref clock property, we would need another way to pass
>> this information to the core driver or another way for the dwc3 core
>> driver to check for specific pci device's properties and quirks.
> 
> We've used the device tree to instantiate/configure devices inside of a PCI
> device, though obviously that only works on DT-based platforms, and for
> hardware that's part of the board itself, not an add-in card.
> 
> We've also used the MFD infrastructure to instantiate devices and device
> properties inside a PCI device on x86, which can be used if the driver you are
> instantiating uses the generic device property accessors and not the DT-
> specific ones. That gets a bit dirty however - I don't think there's an easy
> way to create properties that are references to other nodes, or more than a
> single level deep heirarchy of nodes.
> 
> For a use case like you're describing, it sounds like it would be better to
> abstract away some of the core DWC3 code from reading the settings from DT
> directly, so that the PCI devices can instantiate it and set the configuration
> however they want, without having to worry about creating fake properties for
> the core to read. I think that pattern has been used with some other drivers
> such as AHCI?
> 

Yes. It would be great if we can rework and abstract this. I'd need to
review how AHCI handles it. It doesn't seem like a small task as it
touches on multiple drivers. But if anyone can start this off, I can
help further contribute/review.

Thanks,
Thinh

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

[Sean Anderson]

Hi Thinh,

On 1/18/22 3:00 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>> Hi Thinh,
>>
>> On 1/18/22 2:46 PM, Thinh Nguyen wrote:
>>> Hi Sean,
>>>
>>> Baruch Siach wrote:
>>>> Hi Sean, Thinh,
>>>>
>>>> On Fri, Jan 14 2022, Sean Anderson wrote:
>>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>>>>> program
>>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>>>>> Since
>>>>> we no longer need a special property duplicating this configuration,
>>>>> snps,ref-clock-period-ns is deprecated.
>>>>>
>>>>> Please test this! Patches 3/4 in this series have the effect of
>>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>>>>> configure
>>>>> the "ref" clock. I have build tested, but not much else.
>>>>>
>>>>> [1]
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
>>>>>
>>>>
>>>> Thinh, you suggested the dedicated DT property for the reference clock:
>>>>
>>>>
>>>> https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
>>>>
>>>>
>>>> Can you comment on this series?
>>>>
>>>
>>> Unless there's a good way to pass this information for PCI devices, my
>>> opinion hasn't changed. (Btw, I don't think creating a dummy clock
>>> provider and its dummy ops is a good solution as seems to complicate and
>>> bloat the PCI glue drivers).
>>
>> Can you explain your situation a bit more? I'm not sure how you can
>> access a device tree property but not add a fixed-rate clock.
>>
>> --Sean
>
> Currently for dwc3 pci devices, we have glue drivers that create a
> platform_device with specific properties to pass to the dwc3 core
> driver. Without a ref clock property, we would need another way to pass
> this information to the core driver or another way for the dwc3 core
> driver to check for specific pci device's properties and quirks.

The primary problem with the existing binding is that it does not
contain enough information to calculate the fractional period. With the
frequency, we can calculate the correct values for the registers without
needing an additional binding. So we need to transition to some kind of
frequency-based system. So perhaps we should add a "ref-clock-frequency"
property and use that as a default for when the clock is missing.

--Sean

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

[Thinh Nguyen]

Hi Sean,

Sean Anderson wrote:
> Hi Thinh,
> 
> On 1/18/22 3:00 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>> Hi Thinh,
>>>
>>> On 1/18/22 2:46 PM, Thinh Nguyen wrote:
>>>> Hi Sean,
>>>>
>>>> Baruch Siach wrote:
>>>>> Hi Sean, Thinh,
>>>>>
>>>>> On Fri, Jan 14 2022, Sean Anderson wrote:
>>>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>>>>>> program
>>>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>>>>>> Since
>>>>>> we no longer need a special property duplicating this configuration,
>>>>>> snps,ref-clock-period-ns is deprecated.
>>>>>>
>>>>>> Please test this! Patches 3/4 in this series have the effect of
>>>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>>>>>> configure
>>>>>> the "ref" clock. I have build tested, but not much else.
>>>>>>
>>>>>> [1]
>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
>>>>>>
>>>>>>
>>>>>
>>>>> Thinh, you suggested the dedicated DT property for the reference
>>>>> clock:
>>>>>
>>>>>
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
>>>>>
>>>>>
>>>>>
>>>>> Can you comment on this series?
>>>>>
>>>>
>>>> Unless there's a good way to pass this information for PCI devices, my
>>>> opinion hasn't changed. (Btw, I don't think creating a dummy clock
>>>> provider and its dummy ops is a good solution as seems to complicate
>>>> and
>>>> bloat the PCI glue drivers).
>>>
>>> Can you explain your situation a bit more? I'm not sure how you can
>>> access a device tree property but not add a fixed-rate clock.
>>>
>>> --Sean
>>
>> Currently for dwc3 pci devices, we have glue drivers that create a
>> platform_device with specific properties to pass to the dwc3 core
>> driver. Without a ref clock property, we would need another way to pass
>> this information to the core driver or another way for the dwc3 core
>> driver to check for specific pci device's properties and quirks.
> 
> The primary problem with the existing binding is that it does not
> contain enough information to calculate the fractional period. With the
> frequency, we can calculate the correct values for the registers without
> needing an additional binding. So we need to transition to some kind of
> frequency-based system. So perhaps we should add a "ref-clock-frequency"
> property and use that as a default for when the clock is missing.
> 

Not sure about others, but that's fine with me. The other solution is to
rework the dwc3 drivers as Robert noted, but that requires some work.

Thanks,
Thinh

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

[Robert Hancock]

On Tue, 2022-01-18 at 21:10 +0000, Thinh Nguyen wrote:
> Hi Sean,
> 
> Sean Anderson wrote:
> > Hi Thinh,
> > 
> > On 1/18/22 3:00 PM, Thinh Nguyen wrote:
> > > Sean Anderson wrote:
> > > > Hi Thinh,
> > > > 
> > > > On 1/18/22 2:46 PM, Thinh Nguyen wrote:
> > > > > Hi Sean,
> > > > > 
> > > > > Baruch Siach wrote:
> > > > > > Hi Sean, Thinh,
> > > > > > 
> > > > > > On Fri, Jan 14 2022, Sean Anderson wrote:
> > > > > > > This is a rework of patches 3-5 of [1]. It attempts to correctly
> > > > > > > program
> > > > > > > REFCLKPER and REFCLK_FLADJ based on the reference clock
> > > > > > > frequency.
> > > > > > > Since
> > > > > > > we no longer need a special property duplicating this
> > > > > > > configuration,
> > > > > > > snps,ref-clock-period-ns is deprecated.
> > > > > > > 
> > > > > > > Please test this! Patches 3/4 in this series have the effect of
> > > > > > > programming REFCLKPER and REFCLK_FLADJ on boards which already
> > > > > > > configure
> > > > > > > the "ref" clock. I have build tested, but not much else.
> > > > > > > 
> > > > > > > [1]
> > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbdC4d2R$
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > Thinh, you suggested the dedicated DT property for the reference
> > > > > > clock:
> > > > > > 
> > > > > > 
> > > > > > https://urldefense.com/v3/__https://lore.kernel.org/all/d5acb192-80b9-36f7-43f5-81f21c4e6ba0@synopsys.com/__;!!A4F2R9G_pg!M3zKxDZC9a_etqzXo7GSEMTHRWfc1wR_84wwM4-fShiA35CsGcxcTEffHPbprbpOFmvX$
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Can you comment on this series?
> > > > > > 
> > > > > 
> > > > > Unless there's a good way to pass this information for PCI devices,
> > > > > my
> > > > > opinion hasn't changed. (Btw, I don't think creating a dummy clock
> > > > > provider and its dummy ops is a good solution as seems to complicate
> > > > > and
> > > > > bloat the PCI glue drivers).
> > > > 
> > > > Can you explain your situation a bit more? I'm not sure how you can
> > > > access a device tree property but not add a fixed-rate clock.
> > > > 
> > > > --Sean
> > > 
> > > Currently for dwc3 pci devices, we have glue drivers that create a
> > > platform_device with specific properties to pass to the dwc3 core
> > > driver. Without a ref clock property, we would need another way to pass
> > > this information to the core driver or another way for the dwc3 core
> > > driver to check for specific pci device's properties and quirks.
> > 
> > The primary problem with the existing binding is that it does not
> > contain enough information to calculate the fractional period. With the
> > frequency, we can calculate the correct values for the registers without
> > needing an additional binding. So we need to transition to some kind of
> > frequency-based system. So perhaps we should add a "ref-clock-frequency"
> > property and use that as a default for when the clock is missing.
> > 
> 
> Not sure about others, but that's fine with me. The other solution is to
> rework the dwc3 drivers as Robert noted, but that requires some work.
> 

Seems reasonable enough for a short-term fix anyway.

> Thanks,
> Thinh
> 
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

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

[Jun Li]

Sean Anderson <sean.anderson@seco.com> 于2022年1月15日周六 10:11写道：
>
> This is a rework of patches 3-5 of [1]. It attempts to correctly program
> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
> we no longer need a special property duplicating this configuration,
> snps,ref-clock-period-ns is deprecated.
>
> Please test this! Patches 3/4 in this series have the effect of
> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
> the "ref" clock. I have build tested, but not much else.

DWC3 databook states a *condition* for program those settings:

This field must be programmed to a non-zero value only if
GFLADJ_REFCLK_LPM_SEL is set to '1' or GCTL.SOFITPSYNC is set to '1'.
The value is derived as follows:
FLADJ_REF_CLK_FLADJ=((125000/ref_clk_period_integer)-(125000/ref_clk_period))
* ref_clk_period where
■ the ref_clk_period_integer is the integer value of the ref_clk
period got by truncating the decimal (fractional) value that is
programmed in the GUCTL.REF_CLK_PERIOD field.
■ the ref_clk_period is the ref_clk period including the fractional value.

So you may need a condition check, with that, only required users
are effected even with "ref" clock specified.

Li Jun

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

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

[Robert Hancock]

On Mon, 2022-01-17 at 20:30 +0800, Jun Li wrote:
> Sean Anderson <sean.anderson@seco.com> 于2022年1月15日周六 10:11写道：
> > This is a rework of patches 3-5 of [1]. It attempts to correctly program
> > REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
> > we no longer need a special property duplicating this configuration,
> > snps,ref-clock-period-ns is deprecated.
> > 
> > Please test this! Patches 3/4 in this series have the effect of
> > programming REFCLKPER and REFCLK_FLADJ on boards which already configure
> > the "ref" clock. I have build tested, but not much else.
> 
> DWC3 databook states a *condition* for program those settings:
> 
> This field must be programmed to a non-zero value only if
> GFLADJ_REFCLK_LPM_SEL is set to '1' or GCTL.SOFITPSYNC is set to '1'.
> The value is derived as follows:
> FLADJ_REF_CLK_FLADJ=((125000/ref_clk_period_integer)-(125000/ref_clk_period))
> * ref_clk_period where
> ■ the ref_clk_period_integer is the integer value of the ref_clk
> period got by truncating the decimal (fractional) value that is
> programmed in the GUCTL.REF_CLK_PERIOD field.
> ■ the ref_clk_period is the ref_clk period including the fractional value.
> 
> So you may need a condition check, with that, only required users
> are effected even with "ref" clock specified.
> 

The Xilinx register documentation for this register in the DWC3 core ( 
https://www.xilinx.com/html_docs/registers/ug1087/usb3_xhci___gfladj.html ) has
the same description, but it is rather confusingly worded. I suspect what they
really mean is that "this field only needs to be programmed if
GFLADJ_REFCLK_LPM_SEL is set to '1' or GCTL.SOFITPSYNC is set to '1'", not
"this field should only be programmed if GFLADJ_REFCLK_LPM_SEL is set to '1' or
GCTL.SOFITPSYNC is set to '1'". I'm not sure if someone can confirm that
interpretation is correct?

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

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

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

[Sean Anderson]

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

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

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

I'll add those in v2.

--Sean