mbox series

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

Message ID 20220114233904.907918-1-sean.anderson@seco.com
Headers show
Series usb: dwc3: Calculate REFCLKPER et. al. from reference clock | expand

Message

Sean Anderson Jan. 14, 2022, 11:38 p.m. UTC
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(-)

Comments

Baruch Siach Jan. 17, 2022, 10:36 a.m. UTC | #1
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(-)
Jun Li Jan. 17, 2022, 12:30 p.m. UTC | #2
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
>
Robert Hancock Jan. 17, 2022, 11:49 p.m. UTC | #3
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
> >
Thinh Nguyen Jan. 18, 2022, 7:46 p.m. UTC | #4
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(-)
> 
>
Sean Anderson Jan. 18, 2022, 7:53 p.m. UTC | #5
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
Thinh Nguyen Jan. 18, 2022, 8 p.m. UTC | #6
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
Robert Hancock Jan. 18, 2022, 8:39 p.m. UTC | #7
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
Thinh Nguyen Jan. 18, 2022, 8:54 p.m. UTC | #8
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
Sean Anderson Jan. 18, 2022, 8:59 p.m. UTC | #9
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
Thinh Nguyen Jan. 18, 2022, 9:10 p.m. UTC | #10
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
Sean Anderson Jan. 18, 2022, 11:15 p.m. UTC | #11
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
Robert Hancock Jan. 18, 2022, 11:21 p.m. UTC | #12
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
>