| Message ID | 20220114233904.907918-1-sean.anderson@seco.com |
|---|---|
| Headers | show |
| Series | usb: dwc3: Calculate REFCLKPER et. al. from reference clock | expand |
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(-)
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 >
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 > >
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 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 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
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
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 >