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

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

[Sean Anderson]

This is a rework of patches 3-5 of [1]. It attempts to correctly program
REFCLKPER and REFCLK_FLADJ based on the reference clock frequency. Since
we no longer need a special property duplicating this configuration,
snps,ref-clock-period-ns is deprecated.

Please test this! Patches 3/4 in this series have the effect of
programming REFCLKPER and REFCLK_FLADJ on boards which already configure
the "ref" clock. I have build tested, but not much else.

[1] https://lore.kernel.org/linux-usb/20220114044230.2677283-1-robert.hancock@calian.com/


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

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

-- 
2.25.1

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

[Sean Anderson]

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

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

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

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

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

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

[Sean Anderson]

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

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

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

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
index 1e0b1bca7c94..8493dd7d5f1f 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
@@ -223,11 +223,11 @@ &uart1 {
 	clocks = <&zynqmp_clk UART1_REF>, <&zynqmp_clk LPD_LSBUS>;
 };
 
-&usb0 {
+&dwc3_0 {
 	clocks = <&zynqmp_clk USB0_BUS_REF>, <&zynqmp_clk USB3_DUAL_REF>;
 };
 
-&usb1 {
+&dwc3_1 {
 	clocks = <&zynqmp_clk USB1_BUS_REF>, <&zynqmp_clk USB3_DUAL_REF>;
 };
 
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 74e66443e4ce..ba68fb8529ee 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -811,7 +811,6 @@ usb0: usb@ff9d0000 {
 			status = "disabled";
 			compatible = "xlnx,zynqmp-dwc3";
 			reg = <0x0 0xff9d0000 0x0 0x100>;
-			clock-names = "bus_clk", "ref_clk";
 			power-domains = <&zynqmp_firmware PD_USB_0>;
 			resets = <&zynqmp_reset ZYNQMP_RESET_USB0_CORERESET>,
 				 <&zynqmp_reset ZYNQMP_RESET_USB0_HIBERRESET>,
@@ -825,6 +824,7 @@ dwc3_0: usb@fe200000 {
 				interrupt-parent = <&gic>;
 				interrupt-names = "dwc_usb3", "otg";
 				interrupts = <0 65 4>, <0 69 4>;
+				clock-names = "bus_early", "ref";
 				#stream-id-cells = <1>;
 				iommus = <&smmu 0x860>;
 				snps,quirk-frame-length-adjustment = <0x20>;
@@ -838,7 +838,6 @@ usb1: usb@ff9e0000 {
 			status = "disabled";
 			compatible = "xlnx,zynqmp-dwc3";
 			reg = <0x0 0xff9e0000 0x0 0x100>;
-			clock-names = "bus_clk", "ref_clk";
 			power-domains = <&zynqmp_firmware PD_USB_1>;
 			resets = <&zynqmp_reset ZYNQMP_RESET_USB1_CORERESET>,
 				 <&zynqmp_reset ZYNQMP_RESET_USB1_HIBERRESET>,
@@ -852,6 +851,7 @@ dwc3_1: usb@fe300000 {
 				interrupt-parent = <&gic>;
 				interrupt-names = "dwc_usb3", "otg";
 				interrupts = <0 70 4>, <0 74 4>;
+				clock-names = "bus_early", "ref";
 				#stream-id-cells = <1>;
 				iommus = <&smmu 0x861>;
 				snps,quirk-frame-length-adjustment = <0x20>;
-- 
2.25.1

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

[Sean Anderson]

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

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

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

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

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

[Baruch Siach]

Hi Sean, Thinh,

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

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

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

Can you comment on this series?

Thanks,
baruch

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


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

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

[Jun Li]

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

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

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

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

Li Jun

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

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

[Robert Hancock]

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

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

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

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

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

[Thinh Nguyen]

Hi Sean,

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

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

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

Thanks,
Thinh

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

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

[Sean Anderson]

Hi Thinh,

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

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

--Sean

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

[Thinh Nguyen]

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

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

BR,
Thinh

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

[Robert Hancock]

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

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

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

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

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

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

[Thinh Nguyen]

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

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

Thanks,
Thinh

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

[Sean Anderson]

Hi Thinh,

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

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

--Sean

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

[Thinh Nguyen]

Hi Sean,

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

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

Thanks,
Thinh

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

[Sean Anderson]

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

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

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

I'll add those in v2.

--Sean

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

[Robert Hancock]

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

Seems reasonable enough for a short-term fix anyway.

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