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 v2:
- Document clock members
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a
- Add snps,ref-clock-frequency-hz property for ACPI
Sean Anderson (7):
dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
usb: dwc3: Get clocks individually
usb: dwc3: Calculate REFCLKPER based on reference clock
usb: dwc3: Program GFLADJ
usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
arm64: dts: zynqmp: Move USB clocks to dwc3 node
arm64: dts: ipq6018: Use reference clock to set dwc3 period
.../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
.../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
drivers/usb/dwc3/core.c | 112 +++++++++++++++---
drivers/usb/dwc3/core.h | 17 ++-
6 files changed, 120 insertions(+), 27 deletions(-)
--
2.25.1
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]>
---
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]>
---
(no changes since v1)
drivers/usb/dwc3/core.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 699ab9abdc47..5214daceda86 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,13 +348,22 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
u32 reg;
+ unsigned long rate, period;
- if (dwc->ref_clk_per == 0)
+ if (dwc->ref_clk) {
+ rate = clk_get_rate(dwc->ref_clk);
+ if (!rate)
+ return;
+ period = NSEC_PER_SEC / rate;
+ } else if (dwc->ref_clk_per) {
+ period = dwc->ref_clk_per;
+ } else {
return;
+ }
reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
- reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, dwc->ref_clk_per);
+ reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
}
--
2.25.1
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]>
---
Changes in v2:
- New
drivers/usb/dwc3/core.c | 6 ++++--
drivers/usb/dwc3/core.h | 4 +++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 883e119377f0..5f3dc5f6cbcb 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
u32 reg;
unsigned long decr, fladj, rate, period;
- if (dwc->ref_clk) {
- rate = clk_get_rate(dwc->ref_clk);
+ if (dwc->ref_clk || dwc->ref_clk_freq) {
+ rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
if (!rate)
return;
period = NSEC_PER_SEC / rate;
@@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
&dwc->fladj);
device_property_read_u32(dev, "snps,ref-clock-period-ns",
&dwc->ref_clk_per);
+ device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
+ &dwc->ref_clk_freq);
dwc->dis_metastability_quirk = device_property_read_bool(dev,
"snps,dis_metastability_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eb9c1efced05..00a792459fec 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
* @regs: base address for our registers
* @regs_size: address space size
* @fladj: frame length adjustment
- * @ref_clk_per: reference clock period configuration
+ * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
+ * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
* @irq_gadget: peripheral controller's IRQ number
* @otg_irq: IRQ number for OTG IRQs
* @current_otg_role: current role of operation while using the OTG block
@@ -1171,6 +1172,7 @@ struct dwc3 {
u32 fladj;
u32 ref_clk_per;
+ u32 ref_clk_freq;
u32 irq_gadget;
u32 otg_irq;
u32 current_otg_role;
--
2.25.1
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]>
---
Changes in v2:
- Also program GFLADJ.240MHZDECR
- Don't program GFLADJ if the version is < 2.50a
drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
drivers/usb/dwc3/core.h | 3 +++
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5214daceda86..883e119377f0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
static void dwc3_ref_clk_period(struct dwc3 *dwc)
{
u32 reg;
- unsigned long rate, period;
+ unsigned long decr, fladj, rate, period;
if (dwc->ref_clk) {
rate = clk_get_rate(dwc->ref_clk);
@@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
period = NSEC_PER_SEC / rate;
} else if (dwc->ref_clk_per) {
period = dwc->ref_clk_per;
+ rate = NSEC_PER_SEC / period;
} else {
return;
}
@@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
+
+ if (DWC3_VER_IS_PRIOR(DWC3, 250A))
+ return;
+
+ /*
+ * The calculation below is
+ *
+ * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
+ *
+ * but rearranged for fixed-point arithmetic.
+ *
+ * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
+ * nanoseconds of error caused by the truncation which happened during
+ * the division when calculating rate or period (whichever one was
+ * derived from the other). We first calculate the relative error, then
+ * scale it to units of 0.08%.
+ */
+ fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
+ fladj -= 125000;
+
+ /*
+ * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
+ */
+ decr = 480000000 / rate;
+
+ reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+ reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
+ & ~DWC3_GFLADJ_240MHZDECR
+ & ~DWC3_GFLADJ_240MHZDECR_PLS1;
+ reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
+ | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
+ | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
+ dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
}
-
/**
* dwc3_free_one_event_buffer - Frees one event buffer
* @dwc: Pointer to our controller context structure
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45cfa7d9f27a..eb9c1efced05 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -388,6 +388,9 @@
/* Global Frame Length Adjustment Register */
#define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
#define DWC3_GFLADJ_30MHZ_MASK 0x3f
+#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
+#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
+#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
/* Global User Control Register*/
#define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
--
2.25.1
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
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]>
---
(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
Hi Sean,
On Tue, Jan 18 2022, Sean Anderson wrote:
> This is a rework of patches 3-5 of [1]. It attempts to correctly program
> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
> we no longer need a special property duplicating this configuration,
> snps,ref-clock-period-ns is deprecated.
>
> Please test this! Patches 3/4 in this series have the effect of
> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
> the "ref" clock. I have build tested, but not much else.
Tested here on IPQ6010 based system. USB still works. But the with "ref"
clock at 24MHz, period is calculated as 0x29. Previous
snps,ref-clock-period-ns value used to be 0x32.
Is that expected?
Thanks,
baruch
>
> [1] https://lore.kernel.org/linux-usb/[email protected]/
>
> Changes in v2:
> - Document clock members
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
> - Add snps,ref-clock-frequency-hz property for ACPI
>
> Sean Anderson (7):
> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
> usb: dwc3: Get clocks individually
> usb: dwc3: Calculate REFCLKPER based on reference clock
> usb: dwc3: Program GFLADJ
> usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
> arm64: dts: zynqmp: Move USB clocks to dwc3 node
> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>
> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
> drivers/usb/dwc3/core.c | 112 +++++++++++++++---
> drivers/usb/dwc3/core.h | 17 ++-
> 6 files changed, 120 insertions(+), 27 deletions(-)
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
Hi Baruch,
On 1/19/22 1:14 PM, Baruch Siach wrote:
> Hi Sean,
>
> On Tue, Jan 18 2022, Sean Anderson wrote:
>> This is a rework of patches 3-5 of [1]. It attempts to correctly program
>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>> we no longer need a special property duplicating this configuration,
>> snps,ref-clock-period-ns is deprecated.
>>
>> Please test this! Patches 3/4 in this series have the effect of
>> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>> the "ref" clock. I have build tested, but not much else.
>
> Tested here on IPQ6010 based system. USB still works. But the with "ref"
> clock at 24MHz, period is calculated as 0x29. Previous
> snps,ref-clock-period-ns value used to be 0x32.
>
> Is that expected?
Yes. From the documentation for GFLADJ_REFCLK_240MHZ_DECR:
> Examples:
> If the ref_clk is 24 MHz then
> - GUCTL.REF_CLK_PERIOD = 41
> - GFLADJ.GFLADJ_REFCLK_240MHZ_DECR = 240/24 = 10
And 41 == 0x29.
--Sean
On 2022-01-19 23:44, Baruch Siach wrote:
> Hi Sean,
>
> On Tue, Jan 18 2022, Sean Anderson wrote:
>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>> program
>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>> Since
>> we no longer need a special property duplicating this configuration,
>> snps,ref-clock-period-ns is deprecated.
>>
>> Please test this! Patches 3/4 in this series have the effect of
>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>> configure
>> the "ref" clock. I have build tested, but not much else.
>
> Tested here on IPQ6010 based system. USB still works. But the with
> "ref"
> clock at 24MHz, period is calculated as 0x29. Previous
> snps,ref-clock-period-ns value used to be 0x32.
>
> Is that expected?
>
> Thanks,
> baruch
>
Hi Baruch,
Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly
mentioned as 0x32, which was corrected recently.
Thanks,
Kathiravan T.
>>
>> [1]
>> https://lore.kernel.org/linux-usb/[email protected]/
>>
>> Changes in v2:
>> - Document clock members
>> - Also program GFLADJ.240MHZDECR
>> - Don't program GFLADJ if the version is < 2.50a
>> - Add snps,ref-clock-frequency-hz property for ACPI
>>
>> Sean Anderson (7):
>> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
>> usb: dwc3: Get clocks individually
>> usb: dwc3: Calculate REFCLKPER based on reference clock
>> usb: dwc3: Program GFLADJ
>> usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>> arm64: dts: zynqmp: Move USB clocks to dwc3 node
>> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>>
>> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
>> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
>> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
>> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
>> drivers/usb/dwc3/core.c | 112
>> +++++++++++++++---
>> drivers/usb/dwc3/core.h | 17 ++-
>> 6 files changed, 120 insertions(+), 27 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
Hi Kathiravan,
On Thu, Jan 20 2022, Kathiravan T wrote:
> On 2022-01-19 23:44, Baruch Siach wrote:
>> Hi Sean,
>> On Tue, Jan 18 2022, Sean Anderson wrote:
>>> This is a rework of patches 3-5 of [1]. It attempts to correctly program
>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>>> we no longer need a special property duplicating this configuration,
>>> snps,ref-clock-period-ns is deprecated.
>>> Please test this! Patches 3/4 in this series have the effect of
>>> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>>> the "ref" clock. I have build tested, but not much else.
>> Tested here on IPQ6010 based system. USB still works. But the with
>> "ref"
>> clock at 24MHz, period is calculated as 0x29. Previous
>> snps,ref-clock-period-ns value used to be 0x32.
>> Is that expected?
>
> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
> as 0x32, which was corrected recently.
Thanks for the update. This needs fixing in upstream kernel. I'll send a
patch.
For some reason USB appears to work here with both values. Is it because
I only use USB2 signals? If this is the case them I can not actually
test this series on my system.
Thanks,
baruch
>>> [1]
>>> https://lore.kernel.org/linux-usb/[email protected]/
>>> Changes in v2:
>>> - Document clock members
>>> - Also program GFLADJ.240MHZDECR
>>> - Don't program GFLADJ if the version is < 2.50a
>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>> Sean Anderson (7):
>>> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
>>> usb: dwc3: Get clocks individually
>>> usb: dwc3: Calculate REFCLKPER based on reference clock
>>> usb: dwc3: Program GFLADJ
>>> usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>> arm64: dts: zynqmp: Move USB clocks to dwc3 node
>>> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>>> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
>>> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
>>> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
>>> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
>>> drivers/usb/dwc3/core.c | 112 +++++++++++++++---
>>> drivers/usb/dwc3/core.h | 17 ++-
>>> 6 files changed, 120 insertions(+), 27 deletions(-)
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -
On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> Instead of using a special property to determine the reference clock
> period, use the rate of the reference clock. When we have a legacy
> snps,ref-clock-period-ns property and no reference clock, use it
> instead. Fractional clocks are not currently supported, and will be
> dealt with in the next commit.
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> (no changes since v1)
>
> drivers/usb/dwc3/core.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 699ab9abdc47..5214daceda86 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -348,13 +348,22 @@ static void dwc3_frame_length_adjustment(struct dwc3
> *dwc)
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> u32 reg;
> + unsigned long rate, period;
>
> - if (dwc->ref_clk_per == 0)
> + if (dwc->ref_clk) {
> + rate = clk_get_rate(dwc->ref_clk);
> + if (!rate)
> + return;
> + period = NSEC_PER_SEC / rate;
> + } else if (dwc->ref_clk_per) {
> + period = dwc->ref_clk_per;
> + } else {
> return;
> + }
>
> reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
> - reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, dwc->ref_clk_per);
> + reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> }
>
Looks good here on a ZynqMP board.
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> Instead of grabbing all clocks in bulk, grab them individually. This will
> allow us to get the frequency or otherwise deal with discrete clocks. This
> may break some platforms if they use a clock which doesn't use one of the
> documented names.
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> Changes in v2:
> - Document clock members
>
> drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++--------
> drivers/usb/dwc3/core.h | 10 ++++---
> 2 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f4c09951b517..699ab9abdc47 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -745,6 +745,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> return 0;
> }
>
> +static int dwc3_clk_enable(struct dwc3 *dwc)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(dwc->bus_clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dwc->ref_clk);
> + if (ret)
> + goto disable_bus_clk;
> +
> + ret = clk_prepare_enable(dwc->susp_clk);
> + if (ret)
> + goto disable_ref_clk;
> +
> + return 0;
> +
> +disable_ref_clk:
> + clk_disable_unprepare(dwc->ref_clk);
> +disable_bus_clk:
> + clk_disable_unprepare(dwc->bus_clk);
> + return ret;
> +}
> +
> +static void dwc3_clk_disable(struct dwc3 *dwc)
> +{
> + clk_disable_unprepare(dwc->susp_clk);
> + clk_disable_unprepare(dwc->ref_clk);
> + clk_disable_unprepare(dwc->bus_clk);
> +}
> +
> static void dwc3_core_exit(struct dwc3 *dwc)
> {
> dwc3_event_buffers_cleanup(dwc);
> @@ -758,7 +790,7 @@ static void dwc3_core_exit(struct dwc3 *dwc)
> usb_phy_set_suspend(dwc->usb3_phy, 1);
> phy_power_off(dwc->usb2_generic_phy);
> phy_power_off(dwc->usb3_generic_phy);
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> reset_control_assert(dwc->reset);
> }
>
> @@ -1605,25 +1637,31 @@ static int dwc3_probe(struct platform_device *pdev)
> return PTR_ERR(dwc->reset);
>
> if (dev->of_node) {
> - ret = devm_clk_bulk_get_all(dev, &dwc->clks);
> - if (ret == -EPROBE_DEFER)
> - return ret;
> /*
> * Clocks are optional, but new DT platforms should support all
> * clocks as required by the DT-binding.
> */
> - if (ret < 0)
> - dwc->num_clks = 0;
> - else
> - dwc->num_clks = ret;
> + dwc->bus_clk = devm_clk_get_optional(dev, "bus_early");
> + if (IS_ERR(dwc->bus_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->bus_clk),
> + "could not get bus clock\n");
>
> + dwc->ref_clk = devm_clk_get_optional(dev, "ref");
> + if (IS_ERR(dwc->ref_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->ref_clk),
> + "could not get ref clock\n");
> +
> + dwc->susp_clk = devm_clk_get_optional(dev, "suspend");
> + if (IS_ERR(dwc->susp_clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->susp_clk),
> + "could not get suspend clock\n");
> }
>
> ret = reset_control_deassert(dwc->reset);
> if (ret)
> return ret;
>
> - ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> + ret = dwc3_clk_enable(dwc);
> if (ret)
> goto assert_reset;
>
> @@ -1711,7 +1749,7 @@ static int dwc3_probe(struct platform_device *pdev)
> pm_runtime_disable(&pdev->dev);
>
> disable_clks:
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> assert_reset:
> reset_control_assert(dwc->reset);
>
> @@ -1755,7 +1793,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> if (ret)
> return ret;
>
> - ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
> + ret = dwc3_clk_enable(dwc);
> if (ret)
> goto assert_reset;
>
> @@ -1766,7 +1804,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> return 0;
>
> disable_clks:
> - clk_bulk_disable_unprepare(dwc->num_clks, dwc->clks);
> + dwc3_clk_disable(dwc);
> assert_reset:
> reset_control_assert(dwc->reset);
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e1cc3f7398fb..45cfa7d9f27a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -978,8 +978,9 @@ struct dwc3_scratchpad_array {
> * @eps: endpoint array
> * @gadget: device side representation of the peripheral controller
> * @gadget_driver: pointer to the gadget driver
> - * @clks: array of clocks
> - * @num_clks: number of clocks
> + * @bus_clk: clock for accessing the registers
> + * @ref_clk: reference clock
> + * @susp_clk: clock used when the SS phy is in low power (S3) state
> * @reset: reset control
> * @regs: base address for our registers
> * @regs_size: address space size
> @@ -1134,8 +1135,9 @@ struct dwc3 {
> struct usb_gadget *gadget;
> struct usb_gadget_driver *gadget_driver;
>
> - struct clk_bulk_data *clks;
> - int num_clks;
> + struct clk *bus_clk;
> + struct clk *ref_clk;
> + struct clk *susp_clk;
>
> struct reset_control *reset;
>
Reviewed-by: Robert Hancock <[email protected]>
On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> GUCTL.REFCLKPER can only account for clock frequencies with integer
> periods. To address this, program REFCLK_FLADJ with the relative error
> caused by period truncation. The formula given in the register reference
> has been rearranged to allow calculation based on rate (instead of
> period), and to allow for fixed-point arithmetic.
>
> Additionally, calculate a value for 240MHZDECR. This configures a
> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>
> This register is programmed only for versions >= 2.50a, since this is
> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
> adjustment quirk").
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> Changes in v2:
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
>
> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
> drivers/usb/dwc3/core.h | 3 +++
> 2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5214daceda86..883e119377f0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3
> *dwc)
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> u32 reg;
> - unsigned long rate, period;
> + unsigned long decr, fladj, rate, period;
>
> if (dwc->ref_clk) {
> rate = clk_get_rate(dwc->ref_clk);
> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> period = NSEC_PER_SEC / rate;
> } else if (dwc->ref_clk_per) {
> period = dwc->ref_clk_per;
> + rate = NSEC_PER_SEC / period;
> } else {
> return;
> }
> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> +
> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> + return;
> +
> + /*
> + * The calculation below is
> + *
> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
> + *
> + * but rearranged for fixed-point arithmetic.
> + *
> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
> + * nanoseconds of error caused by the truncation which happened during
> + * the division when calculating rate or period (whichever one was
> + * derived from the other). We first calculate the relative error, then
> + * scale it to units of 0.08%.
> + */
> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
> + fladj -= 125000;
> +
> + /*
> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
> + */
> + decr = 480000000 / rate;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
> + & ~DWC3_GFLADJ_240MHZDECR
> + & ~DWC3_GFLADJ_240MHZDECR_PLS1;
> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> }
>
> -
> /**
> * dwc3_free_one_event_buffer - Frees one event buffer
> * @dwc: Pointer to our controller context structure
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45cfa7d9f27a..eb9c1efced05 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -388,6 +388,9 @@
> /* Global Frame Length Adjustment Register */
> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
> #define DWC3_GFLADJ_30MHZ_MASK 0x3f
> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
>
> /* Global User Control Register*/
> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
Looks good here on a ZynqMP board. GFLADJ_REFCLK_FLADJ ends up being set to 1
now instead of 0, since the clock rate is actually 19999800 Hz rather than 20
MHz. But that should be correct, and USB still seems to work fine.
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
On Tue, 2022-01-18 at 19:24 -0500, Sean Anderson wrote:
> These clocks are not used by the dwc3-xilinx driver except to
> enable/disable them. Move them to the dwc3 node so its driver can use
> them to configure the reference clock period.
>
> Signed-off-by: Sean Anderson <[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>;
Tested and working on ZynqMP.
Reviewed-by: Robert Hancock <[email protected]>
Tested-by: Robert Hancock <[email protected]>
Hi Baruch,
On 1/20/2022 3:59 PM, Baruch Siach wrote:
> Hi Kathiravan,
>
> On Thu, Jan 20 2022, Kathiravan T wrote:
>> On 2022-01-19 23:44, Baruch Siach wrote:
>>> Hi Sean,
>>> On Tue, Jan 18 2022, Sean Anderson wrote:
>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly program
>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
>>>> we no longer need a special property duplicating this configuration,
>>>> snps,ref-clock-period-ns is deprecated.
>>>> Please test this! Patches 3/4 in this series have the effect of
>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already configure
>>>> the "ref" clock. I have build tested, but not much else.
>>> Tested here on IPQ6010 based system. USB still works. But the with
>>> "ref"
>>> clock at 24MHz, period is calculated as 0x29. Previous
>>> snps,ref-clock-period-ns value used to be 0x32.
>>> Is that expected?
>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly mentioned
>> as 0x32, which was corrected recently.
> Thanks for the update. This needs fixing in upstream kernel. I'll send a
> patch.
>
> For some reason USB appears to work here with both values. Is it because
> I only use USB2 signals? If this is the case them I can not actually
> test this series on my system.
I could recollect we did see some issue on USB2.0 port as well, but it
wasn't fatal one. Anyways it is better to test it.
Thanks,
Kathiravan T.
>
> Thanks,
> baruch
>
>>>> [1]
>>>> https://lore.kernel.org/linux-usb/[email protected]/
>>>> Changes in v2:
>>>> - Document clock members
>>>> - Also program GFLADJ.240MHZDECR
>>>> - Don't program GFLADJ if the version is < 2.50a
>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>> Sean Anderson (7):
>>>> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
>>>> usb: dwc3: Get clocks individually
>>>> usb: dwc3: Calculate REFCLKPER based on reference clock
>>>> usb: dwc3: Program GFLADJ
>>>> usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>> arm64: dts: zynqmp: Move USB clocks to dwc3 node
>>>> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>>>> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
>>>> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
>>>> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
>>>> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
>>>> drivers/usb/dwc3/core.c | 112 +++++++++++++++---
>>>> drivers/usb/dwc3/core.h | 17 ++-
>>>> 6 files changed, 120 insertions(+), 27 deletions(-)
>
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]>
> ---
>
> Changes in v2:
> - New
>
> drivers/usb/dwc3/core.c | 6 ++++--
> drivers/usb/dwc3/core.h | 4 +++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 883e119377f0..5f3dc5f6cbcb 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> u32 reg;
> unsigned long decr, fladj, rate, period;
>
> - if (dwc->ref_clk) {
> - rate = clk_get_rate(dwc->ref_clk);
> + if (dwc->ref_clk || dwc->ref_clk_freq) {
> + rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
> if (!rate)
> return;
> period = NSEC_PER_SEC / rate;
> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> &dwc->fladj);
> device_property_read_u32(dev, "snps,ref-clock-period-ns",
> &dwc->ref_clk_per);
> + device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
> + &dwc->ref_clk_freq);
Please also document in dwc3 DT file whenever we add a new property.
Thanks,
Thinh
>
> dwc->dis_metastability_quirk = device_property_read_bool(dev,
> "snps,dis_metastability_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..00a792459fec 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
> * @regs: base address for our registers
> * @regs_size: address space size
> * @fladj: frame length adjustment
> - * @ref_clk_per: reference clock period configuration
> + * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
> + * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
> * @irq_gadget: peripheral controller's IRQ number
> * @otg_irq: IRQ number for OTG IRQs
> * @current_otg_role: current role of operation while using the OTG block
> @@ -1171,6 +1172,7 @@ struct dwc3 {
>
> u32 fladj;
> u32 ref_clk_per;
> + u32 ref_clk_freq;
> u32 irq_gadget;
> u32 otg_irq;
> u32 current_otg_role;
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]>
> ---
>
> Changes in v2:
> - Also program GFLADJ.240MHZDECR
> - Don't program GFLADJ if the version is < 2.50a
>
> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
> drivers/usb/dwc3/core.h | 3 +++
> 2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5214daceda86..883e119377f0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
> static void dwc3_ref_clk_period(struct dwc3 *dwc)
> {
> u32 reg;
> - unsigned long rate, period;
> + unsigned long decr, fladj, rate, period;
Minor style nit: Felipe prefers to keep the declaration on separate
lines, let's keep it consistent with the rest in this driver.
>
> if (dwc->ref_clk) {
> rate = clk_get_rate(dwc->ref_clk);
> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> period = NSEC_PER_SEC / rate;
> } else if (dwc->ref_clk_per) {
> period = dwc->ref_clk_per;
> + rate = NSEC_PER_SEC / period;
> } else {
> return;
> }
> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> +
> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> + return;
> +
> + /*
> + * The calculation below is
> + *
> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
> + *
> + * but rearranged for fixed-point arithmetic.
> + *
> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
> + * nanoseconds of error caused by the truncation which happened during
> + * the division when calculating rate or period (whichever one was
> + * derived from the other). We first calculate the relative error, then
> + * scale it to units of 0.08%.
> + */
> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
Can we rearrange the math so we don't have to operate on different data
type and deal with conversion/truncation?
> + fladj -= 125000;
> +
> + /*
> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
> + */
> + decr = 480000000 / rate;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
> + & ~DWC3_GFLADJ_240MHZDECR
> + & ~DWC3_GFLADJ_240MHZDECR_PLS1;
> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
Does this pass checkpatch?
> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> }
>
> -
> /**
> * dwc3_free_one_event_buffer - Frees one event buffer
> * @dwc: Pointer to our controller context structure
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45cfa7d9f27a..eb9c1efced05 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -388,6 +388,9 @@
> /* Global Frame Length Adjustment Register */
> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
> #define DWC3_GFLADJ_30MHZ_MASK 0x3f
> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
>
> /* Global User Control Register*/
> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
Thanks,
Thinh
Kathiravan Thirumoorthy wrote:
> Hi Baruch,
>
> On 1/20/2022 3:59 PM, Baruch Siach wrote:
>> Hi Kathiravan,
>>
>> On Thu, Jan 20 2022, Kathiravan T wrote:
>>> On 2022-01-19 23:44, Baruch Siach wrote:
>>>> Hi Sean,
>>>> On Tue, Jan 18 2022, Sean Anderson wrote:
>>>>> This is a rework of patches 3-5 of [1]. It attempts to correctly
>>>>> program
>>>>> REFCLKPER and REFCLK_FLADJ based on the reference clock frequency.
>>>>> Since
>>>>> we no longer need a special property duplicating this configuration,
>>>>> snps,ref-clock-period-ns is deprecated.
>>>>> Please test this! Patches 3/4 in this series have the effect of
>>>>> programming REFCLKPER and REFCLK_FLADJ on boards which already
>>>>> configure
>>>>> the "ref" clock. I have build tested, but not much else.
>>>> Tested here on IPQ6010 based system. USB still works. But the with
>>>> "ref"
>>>> clock at 24MHz, period is calculated as 0x29. Previous
>>>> snps,ref-clock-period-ns value used to be 0x32.
>>>> Is that expected?
>>> Yes, it is 0x29 for IPQ60xx based SoCs. In downstream it was wrongly
>>> mentioned
>>> as 0x32, which was corrected recently.
>> Thanks for the update. This needs fixing in upstream kernel. I'll send a
>> patch.
>>
>> For some reason USB appears to work here with both values. Is it because
>> I only use USB2 signals? If this is the case them I can not actually
>> test this series on my system.
The controller uses the GUCTL.REFCLKPER under specific conditions and
settings, and the conditions are different between host mode and device
mode. For example, if you're running as device-mode and not operating in
low power, and or not using periodic endpoints, you're not probably
testing this.
BR,
Thinh
>
> I could recollect we did see some issue on USB2.0 port as well, but it
> wasn't fatal one. Anyways it is better to test it.
>
> Thanks,
>
> Kathiravan T.
>
>>
>> Thanks,
>> baruch
>>
>>>>> [1]
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/__;!!A4F2R9G_pg!MCqqTGYTR42-4_LUHhrSJjkUOkDZKb795I8lB3PY4dB-kVCBnR9YKucgzrwhlj0tZZm5$
>>>>> Changes in v2:
>>>>> - Document clock members
>>>>> - Also program GFLADJ.240MHZDECR
>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>> - Add snps,ref-clock-frequency-hz property for ACPI
>>>>> Sean Anderson (7):
>>>>> dt-bindings: usb: dwc3: Deprecate snps,ref-clock-period-ns
>>>>> usb: dwc3: Get clocks individually
>>>>> usb: dwc3: Calculate REFCLKPER based on reference clock
>>>>> usb: dwc3: Program GFLADJ
>>>>> usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI
>>>>> arm64: dts: zynqmp: Move USB clocks to dwc3 node
>>>>> arm64: dts: ipq6018: Use reference clock to set dwc3 period
>>>>> .../devicetree/bindings/usb/snps,dwc3.yaml | 7 +-
>>>>> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 3 +-
>>>>> .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi | 4 +-
>>>>> arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 4 +-
>>>>> drivers/usb/dwc3/core.c | 112
>>>>> +++++++++++++++---
>>>>> drivers/usb/dwc3/core.h | 17 ++-
>>>>> 6 files changed, 120 insertions(+), 27 deletions(-)
>>
On 1/24/22 5:46 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>> periods. To address this, program REFCLK_FLADJ with the relative error
>> caused by period truncation. The formula given in the register reference
>> has been rearranged to allow calculation based on rate (instead of
>> period), and to allow for fixed-point arithmetic.
>>
>> Additionally, calculate a value for 240MHZDECR. This configures a
>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>
>> This register is programmed only for versions >= 2.50a, since this is
>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>> adjustment quirk").
>>
>> Signed-off-by: Sean Anderson <[email protected]>
>> ---
>>
>> Changes in v2:
>> - Also program GFLADJ.240MHZDECR
>> - Don't program GFLADJ if the version is < 2.50a
>>
>> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>> drivers/usb/dwc3/core.h | 3 +++
>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 5214daceda86..883e119377f0 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> {
>> u32 reg;
>> - unsigned long rate, period;
>> + unsigned long decr, fladj, rate, period;
>
> Minor style nit: Felipe prefers to keep the declaration on separate
> lines, let's keep it consistent with the rest in this driver.
So
unsigned int decr;
unsigned int fladj;
unsigned int rate;
unsigned int period;
?
Frankly that seems rather verbose.
>>
>> if (dwc->ref_clk) {
>> rate = clk_get_rate(dwc->ref_clk);
>> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> period = NSEC_PER_SEC / rate;
>> } else if (dwc->ref_clk_per) {
>> period = dwc->ref_clk_per;
>> + rate = NSEC_PER_SEC / period;
>> } else {
>> return;
>> }
>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>> +
>> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>> + return;
>> +
>> + /*
>> + * The calculation below is
>> + *
>> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
>> + *
>> + * but rearranged for fixed-point arithmetic.
>> + *
>> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
>> + * nanoseconds of error caused by the truncation which happened during
>> + * the division when calculating rate or period (whichever one was
>> + * derived from the other). We first calculate the relative error, then
>> + * scale it to units of 0.08%.
>> + */
>> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
>
> Can we rearrange the math so we don't have to operate on different data
> type and deal with conversion/truncation?
I don't understand what data types you are referring to.
The truncation above (in the calculaion for rate/period) is intentional,
so we can determine the error introduced by representing period using
only ns.
>> + fladj -= 125000;
>> +
>> + /*
>> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>> + */
>> + decr = 480000000 / rate;
>> +
>> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>> + & ~DWC3_GFLADJ_240MHZDECR
>> + & ~DWC3_GFLADJ_240MHZDECR_PLS1;
>> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
>
> Does this pass checkpatch?
Yes.
--Sean
>> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>> }
>>
>> -
>> /**
>> * dwc3_free_one_event_buffer - Frees one event buffer
>> * @dwc: Pointer to our controller context structure
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 45cfa7d9f27a..eb9c1efced05 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -388,6 +388,9 @@
>> /* Global Frame Length Adjustment Register */
>> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
>> #define DWC3_GFLADJ_30MHZ_MASK 0x3f
>> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
>> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
>> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
>>
>> /* Global User Control Register*/
>> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
> Thanks,
> Thinh
>
On 1/24/22 5:44 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>> This property allows setting the reference clock frequency properly for
>> ACPI-based systems. It is not documented under dt-bindings, since it is
>> not intended for use on DT-based systems. DT-based systems should use
>> the clocks property instead.
>>
>> Frequency is preferred over period since it has greater precision when
>> used in calculations.
>>
>> Signed-off-by: Sean Anderson <[email protected]>
>> ---
>>
>> Changes in v2:
>> - New
>>
>> drivers/usb/dwc3/core.c | 6 ++++--
>> drivers/usb/dwc3/core.h | 4 +++-
>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 883e119377f0..5f3dc5f6cbcb 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>> u32 reg;
>> unsigned long decr, fladj, rate, period;
>>
>> - if (dwc->ref_clk) {
>> - rate = clk_get_rate(dwc->ref_clk);
>> + if (dwc->ref_clk || dwc->ref_clk_freq) {
>> + rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>> if (!rate)
>> return;
>> period = NSEC_PER_SEC / rate;
>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>> &dwc->fladj);
>> device_property_read_u32(dev, "snps,ref-clock-period-ns",
>> &dwc->ref_clk_per);
>> + device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>> + &dwc->ref_clk_freq);
>
> Please also document in dwc3 DT file whenever we add a new property.
This is intentionally undocumented, as noted in the commit message.
Rob Herring has said that dt-bindings should only document properties
intended for device-tree.
--Sean
On 1/24/22 6:07 PM, Sean Anderson wrote:
> On 1/24/22 5:44 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>> This property allows setting the reference clock frequency properly for
>>> ACPI-based systems. It is not documented under dt-bindings, since it is
>>> not intended for use on DT-based systems. DT-based systems should use
>>> the clocks property instead.
>>>
>>> Frequency is preferred over period since it has greater precision when
>>> used in calculations.
>>>
>>> Signed-off-by: Sean Anderson <[email protected]>
>>> ---
>>>
>>> Changes in v2:
>>> - New
>>>
>>> drivers/usb/dwc3/core.c | 6 ++++--
>>> drivers/usb/dwc3/core.h | 4 +++-
>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 883e119377f0..5f3dc5f6cbcb 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>> u32 reg;
>>> unsigned long decr, fladj, rate, period;
>>>
>>> - if (dwc->ref_clk) {
>>> - rate = clk_get_rate(dwc->ref_clk);
>>> + if (dwc->ref_clk || dwc->ref_clk_freq) {
>>> + rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>>> if (!rate)
>>> return;
>>> period = NSEC_PER_SEC / rate;
>>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>> &dwc->fladj);
>>> device_property_read_u32(dev, "snps,ref-clock-period-ns",
>>> &dwc->ref_clk_per);
>>> + device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>>> + &dwc->ref_clk_freq);
>>
>> Please also document in dwc3 DT file whenever we add a new property.
>
> This is intentionally undocumented, as noted in the commit message.
> Rob Herring has said that dt-bindings should only document properties
> intended for device-tree.
context: https://lore.kernel.org/all/20181219202734.GA31178@bogus/
This patch was later resubmitted as 24bc6e68efa0 ("serial: sc16is7xx:
Respect clock-frequency property") without the dt-bindings documentation.
+CC Rob if he wants to comment on this specific situation.
--Sean
Sean Anderson wrote:
>
>
> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>>> periods. To address this, program REFCLK_FLADJ with the relative error
>>> caused by period truncation. The formula given in the register reference
>>> has been rearranged to allow calculation based on rate (instead of
>>> period), and to allow for fixed-point arithmetic.
>>>
>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>
>>> This register is programmed only for versions >= 2.50a, since this is
>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>> adjustment quirk").
>>>
>>> Signed-off-by: Sean Anderson <[email protected]>
>>> ---
>>>
>>> Changes in v2:
>>> - Also program GFLADJ.240MHZDECR
>>> - Don't program GFLADJ if the version is < 2.50a
>>>
>>> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>> drivers/usb/dwc3/core.h | 3 +++
>>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 5214daceda86..883e119377f0 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>> {
>>> u32 reg;
>>> - unsigned long rate, period;
>>> + unsigned long decr, fladj, rate, period;
>>
>> Minor style nit: Felipe prefers to keep the declaration on separate
>> lines, let's keep it consistent with the rest in this driver.
>
> So
>
> unsigned int decr;
> unsigned int fladj;
> unsigned int rate;
> unsigned int period;
>
> ?
>
> Frankly that seems rather verbose.
A couple of the benefits of having it like this is to help with viewing
git-blame if we introduce new variables and help with backporting fix
patch a bit simpler. Mainly I'm just following Felipe's style and keep
it consistent in this driver, but I don't think it's a big deal.
>
>>>
>>> if (dwc->ref_clk) {
>>> rate = clk_get_rate(dwc->ref_clk);
>>> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>> period = NSEC_PER_SEC / rate;
>>> } else if (dwc->ref_clk_per) {
>>> period = dwc->ref_clk_per;
>>> + rate = NSEC_PER_SEC / period;
>>> } else {
>>> return;
>>> }
>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>> +
>>> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>> + return;
>>> +
>>> + /*
>>> + * The calculation below is
>>> + *
>>> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
>>> + *
>>> + * but rearranged for fixed-point arithmetic.
>>> + *
>>> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
>>> + * nanoseconds of error caused by the truncation which happened during
>>> + * the division when calculating rate or period (whichever one was
>>> + * derived from the other). We first calculate the relative error, then
>>> + * scale it to units of 0.08%.
>>> + */
>>> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
>>
>> Can we rearrange the math so we don't have to operate on different data
>> type and deal with conversion/truncation?
>
> I don't understand what data types you are referring to.
>
> The truncation above (in the calculaion for rate/period) is intentional,
> so we can determine the error introduced by representing period using
> only ns.
I was wondering if we rearrange the math so we don't need to cast and
use 64-bit here, but that may not be possible. Just computing/reviewing
in my head while accounting for truncation from 64-bit to 32-bit value
is taxing.
>
>>> + fladj -= 125000;
>>> +
>>> + /*
>>> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>>> + */
>>> + decr = 480000000 / rate;
>>> +
>>> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>>> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>>> + & ~DWC3_GFLADJ_240MHZDECR
>>> + & ~DWC3_GFLADJ_240MHZDECR_PLS1;
>>> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
>>
>> Does this pass checkpatch?
>
> Yes.
>
> --Sean
>
>>> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>>> }
>>>
>>> -
>>> /**
>>> * dwc3_free_one_event_buffer - Frees one event buffer
>>> * @dwc: Pointer to our controller context structure
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 45cfa7d9f27a..eb9c1efced05 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -388,6 +388,9 @@
>>> /* Global Frame Length Adjustment Register */
>>> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
>>> #define DWC3_GFLADJ_30MHZ_MASK 0x3f
>>> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
>>> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
>>> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
>>>
>>> /* Global User Control Register*/
>>> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
Thanks,
Thinh
Sean Anderson wrote:
>
>
> On 1/24/22 6:07 PM, Sean Anderson wrote:
>> On 1/24/22 5:44 PM, Thinh Nguyen wrote:
>>> Sean Anderson wrote:
>>>> This property allows setting the reference clock frequency properly for
>>>> ACPI-based systems. It is not documented under dt-bindings, since it is
>>>> not intended for use on DT-based systems. DT-based systems should use
>>>> the clocks property instead.
>>>>
>>>> Frequency is preferred over period since it has greater precision when
>>>> used in calculations.
>>>>
>>>> Signed-off-by: Sean Anderson <[email protected]>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - New
>>>>
>>>> drivers/usb/dwc3/core.c | 6 ++++--
>>>> drivers/usb/dwc3/core.h | 4 +++-
>>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 883e119377f0..5f3dc5f6cbcb 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -350,8 +350,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>> u32 reg;
>>>> unsigned long decr, fladj, rate, period;
>>>>
>>>> - if (dwc->ref_clk) {
>>>> - rate = clk_get_rate(dwc->ref_clk);
>>>> + if (dwc->ref_clk || dwc->ref_clk_freq) {
>>>> + rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>>>> if (!rate)
>>>> return;
>>>> period = NSEC_PER_SEC / rate;
>>>> @@ -1492,6 +1492,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>> &dwc->fladj);
>>>> device_property_read_u32(dev, "snps,ref-clock-period-ns",
>>>> &dwc->ref_clk_per);
>>>> + device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>>>> + &dwc->ref_clk_freq);
>>>
>>> Please also document in dwc3 DT file whenever we add a new property.
>>
>> This is intentionally undocumented, as noted in the commit message.
>> Rob Herring has said that dt-bindings should only document properties
>> intended for device-tree.
>
> context: https://urldefense.com/v3/__https://lore.kernel.org/all/20181219202734.GA31178@bogus/__;!!A4F2R9G_pg!IFaZE73-RQzYnhLSPKuFGqreudBcyVi6T9lGG5byblzoC9i_diaCBe_soAyHPCupua_T$
>
> This patch was later resubmitted as 24bc6e68efa0 ("serial: sc16is7xx:
> Respect clock-frequency property") without the dt-bindings documentation.
>
> +CC Rob if he wants to comment on this specific situation.
>
> --Sean
If this is acceptable, that's great! It opens up more options to the PCI
glue drivers. Maybe we should also document inline for properties that
don't exist in the DT binding.
Thanks,
Thinh
Hi,
Thinh Nguyen <[email protected]> writes:
>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>> Sean Anderson wrote:
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 5214daceda86..883e119377f0 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>> {
>>>> u32 reg;
>>>> - unsigned long rate, period;
>>>> + unsigned long decr, fladj, rate, period;
>>>
>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>> lines, let's keep it consistent with the rest in this driver.
>>
>> So
>>
>> unsigned int decr;
>> unsigned int fladj;
>> unsigned int rate;
>> unsigned int period;
>>
>> ?
>>
>> Frankly that seems rather verbose.
>
> A couple of the benefits of having it like this is to help with viewing
> git-blame if we introduce new variables and help with backporting fix
it also prevents single lines from being constantly modified just
because we're adding a new variable. In the diff, adding a new variable
should look like a new line being added, rather than modified.
> patch a bit simpler. Mainly I'm just following Felipe's style and keep
it's part of the kernel coding style, actually :-)
--
balbi
On 1/24/22 9:11 PM, Thinh Nguyen wrote:
> Sean Anderson wrote:
>>
>>
>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>> Sean Anderson wrote:
>>>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>>>> periods. To address this, program REFCLK_FLADJ with the relative error
>>>> caused by period truncation. The formula given in the register reference
>>>> has been rearranged to allow calculation based on rate (instead of
>>>> period), and to allow for fixed-point arithmetic.
>>>>
>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>
>>>> This register is programmed only for versions >= 2.50a, since this is
>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>> adjustment quirk").
>>>>
>>>> Signed-off-by: Sean Anderson <[email protected]>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Also program GFLADJ.240MHZDECR
>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>
>>>> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>> drivers/usb/dwc3/core.h | 3 +++
>>>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 5214daceda86..883e119377f0 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>> {
>>>> u32 reg;
>>>> - unsigned long rate, period;
>>>> + unsigned long decr, fladj, rate, period;
>>>
>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>> lines, let's keep it consistent with the rest in this driver.
>>
>> So
>>
>> unsigned int decr;
>> unsigned int fladj;
>> unsigned int rate;
>> unsigned int period;
>>
>> ?
>>
>> Frankly that seems rather verbose.
>
> A couple of the benefits of having it like this is to help with viewing
> git-blame if we introduce new variables and help with backporting fix
> patch a bit simpler. Mainly I'm just following Felipe's style and keep
> it consistent in this driver, but I don't think it's a big deal.
*shrug*
If it's the subsystem style I will rewrite it.
(btw is this documented anywhere for future contributors?)
>>
>>>>
>>>> if (dwc->ref_clk) {
>>>> rate = clk_get_rate(dwc->ref_clk);
>>>> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>> period = NSEC_PER_SEC / rate;
>>>> } else if (dwc->ref_clk_per) {
>>>> period = dwc->ref_clk_per;
>>>> + rate = NSEC_PER_SEC / period;
>>>> } else {
>>>> return;
>>>> }
>>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>>> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>>> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>>> +
>>>> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>>> + return;
>>>> +
>>>> + /*
>>>> + * The calculation below is
>>>> + *
>>>> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
>>>> + *
>>>> + * but rearranged for fixed-point arithmetic.
>>>> + *
>>>> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
>>>> + * nanoseconds of error caused by the truncation which happened during
>>>> + * the division when calculating rate or period (whichever one was
>>>> + * derived from the other). We first calculate the relative error, then
>>>> + * scale it to units of 0.08%.
>>>> + */
>>>> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
>>>
>>> Can we rearrange the math so we don't have to operate on different data
>>> type and deal with conversion/truncation?
>>
>> I don't understand what data types you are referring to.
>>
>> The truncation above (in the calculaion for rate/period) is intentional,
>> so we can determine the error introduced by representing period using
>> only ns.
>
> I was wondering if we rearrange the math so we don't need to cast and
> use 64-bit here, but that may not be possible. Just computing/reviewing
> in my head while accounting for truncation from 64-bit to 32-bit value
> is taxing.
Well the primary issue is that log_2(125000ULL * NSEC_PER_SEC) ~= 49, so
we have to use 64-bit integers if we don't want to do any shifting of
the numerator. It might be possible to analyze the significant bits
through the calculation and determine that we can use less precision for
some of the intermediate calculations, but I haven't done that analysis.
IMO that sort of transformation would likely make the calculations more
difficult to understand, not less.
>>
>>>> + fladj -= 125000;
>>>> +
>>>> + /*
>>>> + * The documented 240MHz constant is scaled by 2 to get PLS1 as well.
>>>> + */
>>>> + decr = 480000000 / rate;
>>>> +
>>>> + reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>>>> + reg &= ~DWC3_GFLADJ_REFCLK_FLADJ_MASK
>>>> + & ~DWC3_GFLADJ_240MHZDECR
>>>> + & ~DWC3_GFLADJ_240MHZDECR_PLS1;
>>>> + reg |= FIELD_PREP(DWC3_GFLADJ_REFCLK_FLADJ_MASK, fladj)
>>>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR, decr >> 1)
>>>> + | FIELD_PREP(DWC3_GFLADJ_240MHZDECR_PLS1, decr & 1);
>>>
>>> Does this pass checkpatch?
>>
>> Yes.
>>
>> --Sean
>>
>>>> + dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
>>>> }
>>>>
>>>> -
>>>> /**
>>>> * dwc3_free_one_event_buffer - Frees one event buffer
>>>> * @dwc: Pointer to our controller context structure
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 45cfa7d9f27a..eb9c1efced05 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -388,6 +388,9 @@
>>>> /* Global Frame Length Adjustment Register */
>>>> #define DWC3_GFLADJ_30MHZ_SDBND_SEL BIT(7)
>>>> #define DWC3_GFLADJ_30MHZ_MASK 0x3f
>>>> +#define DWC3_GFLADJ_REFCLK_FLADJ_MASK GENMASK(21, 8)
>>>> +#define DWC3_GFLADJ_240MHZDECR GENMASK(30, 24)
>>>> +#define DWC3_GFLADJ_240MHZDECR_PLS1 BIT(31)
>>>>
>>>> /* Global User Control Register*/
>>>> #define DWC3_GUCTL_REFCLKPER_MASK 0xffc00000
>
>
> Thanks,
> Thinh
>
Sean Anderson wrote:
>
>
> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>> Sean Anderson wrote:
>>>
>>>
>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>> Sean Anderson wrote:
>>>>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>>>>> periods. To address this, program REFCLK_FLADJ with the relative error
>>>>> caused by period truncation. The formula given in the register reference
>>>>> has been rearranged to allow calculation based on rate (instead of
>>>>> period), and to allow for fixed-point arithmetic.
>>>>>
>>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>>
>>>>> This register is programmed only for versions >= 2.50a, since this is
>>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>>> adjustment quirk").
>>>>>
>>>>> Signed-off-by: Sean Anderson <[email protected]>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Also program GFLADJ.240MHZDECR
>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>>
>>>>> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>> drivers/usb/dwc3/core.h | 3 +++
>>>>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>> {
>>>>> u32 reg;
>>>>> - unsigned long rate, period;
>>>>> + unsigned long decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>>
>>> So
>>>
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>>
>>> ?
>>>
>>> Frankly that seems rather verbose.
>>
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>> it consistent in this driver, but I don't think it's a big deal.
>
> *shrug*
>
> If it's the subsystem style I will rewrite it.
>
Felipe also prefers Reverse Christmas Tree style.
> (btw is this documented anywhere for future contributors?)
>
I don't think it's documented, but Felipe Nak'ed patches that don't
follow this style before. I don't want this to be the main focus of the
conversation for this patch, but I don't want it to go unnoticed either.
Your concern is not alone regarding document (or the lacking of) for
different subsystem-specific styles.
(see https://lwn.net/Articles/758552/)
>>>
>>>>>
>>>>> if (dwc->ref_clk) {
>>>>> rate = clk_get_rate(dwc->ref_clk);
>>>>> @@ -357,6 +357,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>> period = NSEC_PER_SEC / rate;
>>>>> } else if (dwc->ref_clk_per) {
>>>>> period = dwc->ref_clk_per;
>>>>> + rate = NSEC_PER_SEC / period;
>>>>> } else {
>>>>> return;
>>>>> }
>>>>> @@ -365,9 +366,41 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>> reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
>>>>> reg |= FIELD_PREP(DWC3_GUCTL_REFCLKPER_MASK, period);
>>>>> dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
>>>>> +
>>>>> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>>>>> + return;
>>>>> +
>>>>> + /*
>>>>> + * The calculation below is
>>>>> + *
>>>>> + * 125000 * (NSEC_PER_SEC / (rate * period) - 1)
>>>>> + *
>>>>> + * but rearranged for fixed-point arithmetic.
>>>>> + *
>>>>> + * Note that rate * period ~= NSEC_PER_SECOND, minus the number of
>>>>> + * nanoseconds of error caused by the truncation which happened during
>>>>> + * the division when calculating rate or period (whichever one was
>>>>> + * derived from the other). We first calculate the relative error, then
>>>>> + * scale it to units of 0.08%.
>>>>> + */
>>>>> + fladj = div64_u64(125000ULL * NSEC_PER_SEC, (u64)rate * period);
>>>>
>>>> Can we rearrange the math so we don't have to operate on different data
>>>> type and deal with conversion/truncation?
>>>
>>> I don't understand what data types you are referring to.
>>>
>>> The truncation above (in the calculaion for rate/period) is intentional,
>>> so we can determine the error introduced by representing period using
>>> only ns.
>>
>> I was wondering if we rearrange the math so we don't need to cast and
>> use 64-bit here, but that may not be possible. Just computing/reviewing
>> in my head while accounting for truncation from 64-bit to 32-bit value
>> is taxing.
>
> Well the primary issue is that log_2(125000ULL * NSEC_PER_SEC) ~= 49, so
> we have to use 64-bit integers if we don't want to do any shifting of
> the numerator. It might be possible to analyze the significant bits
> through the calculation and determine that we can use less precision for
> some of the intermediate calculations, but I haven't done that analysis.
> IMO that sort of transformation would likely make the calculations more
> difficult to understand, not less.
Fair enough.
Thanks,
Thinh
Felipe Balbi wrote:
>
> Hi,
>
> Thinh Nguyen <[email protected]> writes:
>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>> Sean Anderson wrote:
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>> {
>>>>> u32 reg;
>>>>> - unsigned long rate, period;
>>>>> + unsigned long decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>>
>>> So
>>>
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>>
>>> ?
>>>
>>> Frankly that seems rather verbose.
>>
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
>
> it also prevents single lines from being constantly modified just
> because we're adding a new variable. In the diff, adding a new variable
> should look like a new line being added, rather than modified.
>
Right.
>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>
> it's part of the kernel coding style, actually :-)
>
Glad to see you're here :)
BR,
Thinh
Hi,
Sean Anderson <[email protected]> writes:
> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 5214daceda86..883e119377f0 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>> {
>>>>> u32 reg;
>>>>> - unsigned long rate, period;
>>>>> + unsigned long decr, fladj, rate, period;
>>>>
>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>> lines, let's keep it consistent with the rest in this driver.
>>>
>>> So
>>>
>>> unsigned int decr;
>>> unsigned int fladj;
>>> unsigned int rate;
>>> unsigned int period;
>>>
>>> ?
>>>
>>> Frankly that seems rather verbose.
>>
>> A couple of the benefits of having it like this is to help with viewing
>> git-blame if we introduce new variables and help with backporting fix
>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>> it consistent in this driver, but I don't think it's a big deal.
>
> *shrug*
>
> If it's the subsystem style I will rewrite it.
>
> (btw is this documented anywhere for future contributors?)
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
"To this end, use just one data declaration per line (no commas for
multiple data declarations)"
--
balbi
Hi,
Thinh Nguyen <[email protected]> writes:
>>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>>> Sean Anderson wrote:
>>>>>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>>>>>> periods. To address this, program REFCLK_FLADJ with the relative error
>>>>>> caused by period truncation. The formula given in the register reference
>>>>>> has been rearranged to allow calculation based on rate (instead of
>>>>>> period), and to allow for fixed-point arithmetic.
>>>>>>
>>>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>>>
>>>>>> This register is programmed only for versions >= 2.50a, since this is
>>>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>>>> adjustment quirk").
>>>>>>
>>>>>> Signed-off-by: Sean Anderson <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Also program GFLADJ.240MHZDECR
>>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>>>
>>>>>> drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>>> drivers/usb/dwc3/core.h | 3 +++
>>>>>> 2 files changed, 38 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 5214daceda86..883e119377f0 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>>> {
>>>>>> u32 reg;
>>>>>> - unsigned long rate, period;
>>>>>> + unsigned long decr, fladj, rate, period;
>>>>>
>>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>>> lines, let's keep it consistent with the rest in this driver.
>>>>
>>>> So
>>>>
>>>> unsigned int decr;
>>>> unsigned int fladj;
>>>> unsigned int rate;
>>>> unsigned int period;
>>>>
>>>> ?
>>>>
>>>> Frankly that seems rather verbose.
>>>
>>> A couple of the benefits of having it like this is to help with viewing
>>> git-blame if we introduce new variables and help with backporting fix
>>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>>> it consistent in this driver, but I don't think it's a big deal.
>>
>> *shrug*
>>
>> If it's the subsystem style I will rewrite it.
>>
>
> Felipe also prefers Reverse Christmas Tree style.
that's also part of the coding style and probably documented somewhere
--
balbi
On 1/26/22 5:53 AM, Felipe Balbi wrote:
>
> Hi,
>
> Sean Anderson <[email protected]> writes:
>
>> On 1/24/22 9:11 PM, Thinh Nguyen wrote:
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 5214daceda86..883e119377f0 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>>> static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>>> {
>>>>>> u32 reg;
>>>>>> - unsigned long rate, period;
>>>>>> + unsigned long decr, fladj, rate, period;
>>>>>
>>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>>> lines, let's keep it consistent with the rest in this driver.
>>>>
>>>> So
>>>>
>>>> unsigned int decr;
>>>> unsigned int fladj;
>>>> unsigned int rate;
>>>> unsigned int period;
>>>>
>>>> ?
>>>>
>>>> Frankly that seems rather verbose.
>>>
>>> A couple of the benefits of having it like this is to help with viewing
>>> git-blame if we introduce new variables and help with backporting fix
>>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>>> it consistent in this driver, but I don't think it's a big deal.
>>
>> *shrug*
>>
>> If it's the subsystem style I will rewrite it.
>>
>> (btw is this documented anywhere for future contributors?)
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
>
> "To this end, use just one data declaration per line (no commas for
> multiple data declarations)"
>
This is just if you want to add comments.
--Sean