This is a rework of patches 3-5 of [1]. It attempts to correctly program
REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
we no longer need a special property duplicating this configuration,
snps,ref-clock-period-ns is deprecated.
Please test this! Patches 3/4 in this series have the effect of
programming REFCLKPER and REFCLK_FLADJ on boards which already configure
the "ref" clock. I have build tested, but not much else.
[1] https://lore.kernel.org/linux-usb/[email protected]/
Changes in v3:
- Define each variable on its own line
- Rebase onto linux/master
- Update comment to notes some things mentioned during review
Changes in v2:
- Add snps,ref-clock-frequency-hz property for ACPI
- Also program GFLADJ.240MHZDECR
- Document clock members
- Don't program GFLADJ if the version is < 2.50a
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 | 117 +++++++++++++++---
drivers/usb/dwc3/core.h | 17 ++-
6 files changed, 125 insertions(+), 27 deletions(-)
--
2.25.1
This property is redundant because we can determine the correct value for
REFCLKPER based on the "ref" clock. Deprecate it, and encourage users to
provide a clock instead. This also restricts the minimum and maximum to the
values documented in the register reference [1].
[1] https://www.xilinx.com/html_docs/registers/ug1087/usb3_xhci___guctl.html
Signed-off-by: Sean Anderson <[email protected]>
---
(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
Instead of grabbing all clocks in bulk, grab them individually. This will
allow us to get the frequency or otherwise deal with discrete clocks. This
may break some platforms if they use a clock which doesn't use one of the
documented names.
Signed-off-by: Sean Anderson <[email protected]>
Reviewed-by: Robert Hancock <[email protected]>
---
(no changes since v2)
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
Instead of using a special property to determine the reference clock
period, use the rate of the reference clock. When we have a legacy
snps,ref-clock-period-ns property and no reference clock, use it
instead. Fractional clocks are not currently supported, and will be
dealt with in the next commit.
Signed-off-by: Sean Anderson <[email protected]>
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
---
Changes in v3:
- Define each variable on its own line
drivers/usb/dwc3/core.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 699ab9abdc47..38fef5c74359 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -347,14 +347,24 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
*/
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
+ unsigned long period;
+ unsigned long rate;
u32 reg;
- 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
These clocks are not used by the dwc3-xilinx driver except to
enable/disable them. Move them to the dwc3 node so its driver can use
them to configure the reference clock period.
Signed-off-by: Sean Anderson <[email protected]>
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
---
(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
Instead of manually setting snps,ref-clock-period-ns, we can let the
driver calculate it automatically from the "ref" clock. I haven't
reviewed this board's schematics, so please let me know if this is the
wrong 24MHz clock to use.
Signed-off-by: Sean Anderson <[email protected]>
---
(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
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 <[email protected]>
---
(no changes since v2)
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 18adddfba3da..c1b045121672 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -353,8 +353,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
unsigned long rate;
u32 reg;
- 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;
@@ -1497,6 +1497,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
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 <[email protected]>
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
---
Changes in v3:
- Define each variable on its own line
- Update comment to notes some things mentioned during review
Changes in v2:
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a
drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
drivers/usb/dwc3/core.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 38fef5c74359..18adddfba3da 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,6 +348,8 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
unsigned long period;
+ unsigned long fladj;
+ unsigned long decr;
unsigned long rate;
u32 reg;
@@ -358,6 +360,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;
}
@@ -366,9 +369,43 @@ 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. The division must be
+ * 64-bit because 125000 * NSEC_PER_SEC doesn't fit in 32 bits (and
+ * neither does rate * period).
+ *
+ * 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 8 ppm.
+ */
+ 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
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 <[email protected]>
> Reviewed-by: Robert Hancock <[email protected]>
> Tested-by: Robert Hancock <[email protected]>
> ---
>
> Changes in v3:
> - Define each variable on its own line
> - Update comment to notes some things mentioned during review
>
> Changes in v2:
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
>
> drivers/usb/dwc3/core.c | 39 ++++++++++++++++++++++++++++++++++++++-
> drivers/usb/dwc3/core.h | 3 +++
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 38fef5c74359..18adddfba3da 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -348,6 +348,8 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> unsigned long period;
> + unsigned long fladj;
> + unsigned long decr;
> unsigned long rate;
> u32 reg;
>
> @@ -358,6 +360,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;
> }
> @@ -366,9 +369,43 @@ 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. The division must be
> + * 64-bit because 125000 * NSEC_PER_SEC doesn't fit in 32 bits (and
> + * neither does rate * period).
> + *
> + * 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 8 ppm.
> + */
> + 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
Reviewed-by: Thinh Nguyen <[email protected]>
Thanks for the work,
Thinh
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 <[email protected]>
> Reviewed-by: Robert Hancock <[email protected]>
> Tested-by: Robert Hancock <[email protected]>
> ---
>
> Changes in v3:
> - Define each variable on its own line
>
> drivers/usb/dwc3/core.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 699ab9abdc47..38fef5c74359 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -347,14 +347,24 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
> */
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> + unsigned long period;
> + unsigned long rate;
> u32 reg;
>
> - 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);
> }
>
Reviewed-by: Thinh Nguyen <[email protected]>
Thanks,
Thinh
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 <[email protected]>
> ---
>
> (no changes since v2)
>
> 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 18adddfba3da..c1b045121672 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -353,8 +353,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> unsigned long rate;
> u32 reg;
>
> - 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;
> @@ -1497,6 +1497,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;
Reviewed-by: Thinh Nguyen <[email protected]>
Thanks,
Thinh
On 2/4/22 1:38 PM, Andy Shevchenko wrote:
> On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <[email protected]> wrote:
>> On 2/4/22 7:54 AM, Andy Shevchenko wrote:
>
> ...
>
>> > On Thursday, January 27, 2022, Sean Anderson <[email protected] <mailto:[email protected]>> wrote:
>
>> > Is it function or interface clock?
>> >
>> > We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?
>>
>> I believe this is a "functional" clock. It it is a reference for the USB
>> signals. I'm not sure exactly what the purpose of this clock is, since I
>> do not have access to the databook for this IP. I considered using
>> "clock-frequency", but I am concerned about ambiguity because there is a
>> second "suspend" clock which is also a "functional" clock. The latter
>> clock appears to be used when the PHY is shut down (and not necessarily
>> corresponding to Linux's notion of a suspended device). If it is
>> necessary in the future to configure that clock on ACPI platforms (e.g.
>> to set GCTL.PWRDNSCALE) I think it is clear what the property name would
>> be (snps,susp-clock-frequency-hz).
>
> In order to have more or less unified APIs in the future I would
> suggest using 'clock-frequency' for the "main" functional clock. For
> example, 8250_dw uses it for the baud rate generator, while it also
> utilizes auxiliary clock(s) on some platforms.
>
OK, I had a look though that driver, and it seems like it uses
clock-frequency only for the baudclk, and e.g. apb_pclk has no
corresponding frequency property. For this driver, it would mean that
the suspend clock would only be configurable through device tree. Is
that the approach you recommend?
--Sean
Hi Andy,
On 2/4/22 7:54 AM, Andy Shevchenko wrote:
>
>
> On Thursday, January 27, 2022, Sean Anderson <[email protected] <mailto:[email protected]>> 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
>
> Is it function or interface clock?
>
> We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?
I believe this is a "functional" clock. It it is a reference for the USB
signals. I'm not sure exactly what the purpose of this clock is, since I
do not have access to the databook for this IP. I considered using
"clock-frequency", but I am concerned about ambiguity because there is a
second "suspend" clock which is also a "functional" clock. The latter
clock appears to be used when the PHY is shut down (and not necessarily
corresponding to Linux's notion of a suspended device). If it is
necessary in the future to configure that clock on ACPI platforms (e.g.
to set GCTL.PWRDNSCALE) I think it is clear what the property name would
be (snps,susp-clock-frequency-hz).
--Sean
> Signed-off-by: Sean Anderson <[email protected] <mailto:[email protected]>>
> ---
>
> (no changes since v2)
>
> 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 18adddfba3da..c1b045121672 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -353,8 +353,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> unsigned long rate;
> u32 reg;
>
> - 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;
> @@ -1497,6 +1497,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
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <[email protected]> wrote:
> On 2/4/22 7:54 AM, Andy Shevchenko wrote:
...
> > On Thursday, January 27, 2022, Sean Anderson <[email protected] <mailto:[email protected]>> wrote:
> > Is it function or interface clock?
> >
> > We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?
>
> I believe this is a "functional" clock. It it is a reference for the USB
> signals. I'm not sure exactly what the purpose of this clock is, since I
> do not have access to the databook for this IP. I considered using
> "clock-frequency", but I am concerned about ambiguity because there is a
> second "suspend" clock which is also a "functional" clock. The latter
> clock appears to be used when the PHY is shut down (and not necessarily
> corresponding to Linux's notion of a suspended device). If it is
> necessary in the future to configure that clock on ACPI platforms (e.g.
> to set GCTL.PWRDNSCALE) I think it is clear what the property name would
> be (snps,susp-clock-frequency-hz).
In order to have more or less unified APIs in the future I would
suggest using 'clock-frequency' for the "main" functional clock. For
example, 8250_dw uses it for the baud rate generator, while it also
utilizes auxiliary clock(s) on some platforms.
--
With Best Regards,
Andy Shevchenko
On Fri, Feb 4, 2022 at 8:44 PM Sean Anderson <[email protected]> wrote:
> On 2/4/22 1:38 PM, Andy Shevchenko wrote:
> > On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <[email protected]> wrote:
> >> On 2/4/22 7:54 AM, Andy Shevchenko wrote:
> >> > On Thursday, January 27, 2022, Sean Anderson <[email protected] <mailto:[email protected]>> wrote:
...
> > In order to have more or less unified APIs in the future I would
> > suggest using 'clock-frequency' for the "main" functional clock. For
> > example, 8250_dw uses it for the baud rate generator, while it also
> > utilizes auxiliary clock(s) on some platforms.
>
> OK, I had a look though that driver, and it seems like it uses
> clock-frequency only for the baudclk, and e.g. apb_pclk has no
> corresponding frequency property. For this driver, it would mean that
> the suspend clock would only be configurable through device tree. Is
> that the approach you recommend?
What I meant is to use the "clock-frequency" property as it is kinda
standard de facto for the "main'' functional clock. The rest is up to
the individual drivers. From the API perspective I would expect a
common helper in the future that takes clock name and returns rate
based on clock (if found) or "clock-frequency" property. We may also
extend in far future to support any combinations of the [clock name,
property name] to get a clock rated either via Device Tree or via
ACPI.
--
With Best Regards,
Andy Shevchenko
On Thu, 27 Jan 2022 15:06:30 -0500, Sean Anderson wrote:
> This property is redundant because we can determine the correct value for
> REFCLKPER based on the "ref" clock. Deprecate it, and encourage users to
> provide a clock instead. This also restricts the minimum and maximum to the
> values documented in the register reference [1].
>
> [1] https://www.xilinx.com/html_docs/registers/ug1087/usb3_xhci___guctl.html
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> (no changes since v1)
>
> Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>