| Message ID | 1524135818-14825-3-git-send-email-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Historically, the clocks and resets are handled on the glue layer > side instead of the DWC3 core. For simple cases, dwc3-of-simple.c > takes care of arbitrary number of clocks and resets. The DT node > structure typically looks like as follows: > > dwc3-glue { > compatible = "foo,dwc3"; > clocks = ...; > resets = ...; > ... > > dwc3 { > compatible = "snps,dwc3"; > ... > }; > } > > By supporting the clocks and the reset in the dwc3/core.c, it will > be turned into a single node: > > dwc3 { > compatible = "foo,dwc3", "snps,dwc3"; > clocks = ...; > resets = ...; > ... > } > > This commit adds the binding of clocks and resets specific to this IP. > The number of clocks should generally be the same across SoCs, it is > just some SoCs either tie clocks together or do not provide software > control of some of the clocks. > > I took the clock names from the Synopsys datasheet: "ref" (ref_clk), > "bus_early" (bus_clk_early), and "suspend" (suspend_clk). looking at the code: this could mean that dwc3-exynos.c can be removed mid-term (assuming the PHY and regulator handling can be moved/removed/changed) does the datasheet state anything about the clock speeds? from Documentation/devicetree/bindings/usb/dwc3-xilinx.txt: "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation and >= 60MHz for HS operation > I found only one reset line in the datasheet, hence the reset-names > property is omitted. does the datasheet state whether this is a level or a pulsed reset line? on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared) reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for shared and pulsed reset lines") because the reset line is shared between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...) your current approach (having a vendor-specific "foo,dwc3" binding along with the generic "snps,dwc3") would allow having per-"of_device_id" settings which could indicate whether the reset lines are level or pulsed reset if these are "implementation specific" > Supporting those clocks and resets is the requirement for new platforms. just to confirm: with this series your goal is to replace the wrapper node which is needed due to dwc3-of-simple.c ? would other drivers which currently create a wrapper node (like dwc3-keystone.c) keep their wrapper node or do you have any plans for removing it for the other "wrapper" drivers too? > Enforcing the new binding breaks existing platforms since they specify > clocks and resets in their glue layer node, but nothing in the core > node. I listed such exceptional cases in the DT binding. The driver > code is loosened up to accept no clock/reset. This change is based > on the discussion [1]. (snip) Regards Martin -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-24 2:44 GMT+09:00 Martin Blumenstingl <martin.blumenstingl@googlemail.com>: > Hello, > > On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Historically, the clocks and resets are handled on the glue layer >> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c >> takes care of arbitrary number of clocks and resets. The DT node >> structure typically looks like as follows: >> >> dwc3-glue { >> compatible = "foo,dwc3"; >> clocks = ...; >> resets = ...; >> ... >> >> dwc3 { >> compatible = "snps,dwc3"; >> ... >> }; >> } >> >> By supporting the clocks and the reset in the dwc3/core.c, it will >> be turned into a single node: >> >> dwc3 { >> compatible = "foo,dwc3", "snps,dwc3"; >> clocks = ...; >> resets = ...; >> ... >> } >> >> This commit adds the binding of clocks and resets specific to this IP. >> The number of clocks should generally be the same across SoCs, it is >> just some SoCs either tie clocks together or do not provide software >> control of some of the clocks. >> >> I took the clock names from the Synopsys datasheet: "ref" (ref_clk), >> "bus_early" (bus_clk_early), and "suspend" (suspend_clk). > looking at the code: this could mean that dwc3-exynos.c can be removed > mid-term (assuming the PHY and regulator handling can be > moved/removed/changed) That is a good news. > does the datasheet state anything about the clock speeds? from > Documentation/devicetree/bindings/usb/dwc3-xilinx.txt: > "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation > and >= 60MHz for HS operation Not sure. "xlnx,zynqmp-dwc3" is a member of dwc3-of-simple.c which just enables/disables clocks. That information is not important. >> I found only one reset line in the datasheet, hence the reset-names >> property is omitted. > does the datasheet state whether this is a level or a pulsed reset line? > on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared) > reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for > shared and pulsed reset lines") because the reset line is shared > between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...) > your current approach (having a vendor-specific "foo,dwc3" binding > along with the generic "snps,dwc3") would allow having > per-"of_device_id" settings which could indicate whether the reset > lines are level or pulsed reset if these are "implementation specific" I guess your commit ff0a632f08759e31f45b06fee27bc71c826c6b11 is wrong. About the clocks, Rob Herring pointed out: However, this should be specific as to how many clocks and their function. This should be readily available to someone with access to Synopsys datasheet. The number of clocks should generally be the same across SoCs, it is just some SoCs either tie clocks together or don't provide s/w control of some of the clocks. The same applies to the reset control. The reset policy should be the same across SoCs since we are using the same IP (i.e. the same delivered RTL). You are using a pulse reset for DWC3 just because the reset controller in your SoC is implemented like that. This is NOT because your DWC3 in your SoC is specially customized, right? You put a reset-provider matter to a reset-consumer. >> Supporting those clocks and resets is the requirement for new platforms. > just to confirm: > with this series your goal is to replace the wrapper node which is > needed due to dwc3-of-simple.c ? In my _hope_. But, I cannot do this by myself since I do not have such boards. Depends on how people make efforts to covert existing platforms. > would other drivers which currently create a wrapper node (like > dwc3-keystone.c) keep their wrapper node or do you have any plans for > removing it for the other "wrapper" drivers too? We need to take a close look per driver. Looking at the dwc3-keystone.c, it works like an interrupt controller with irq-domain hierarchy. If it is moved to the irqchip subsystem, dwc3-keystone.c could be removed. >> Enforcing the new binding breaks existing platforms since they specify >> clocks and resets in their glue layer node, but nothing in the core >> node. I listed such exceptional cases in the DT binding. The driver >> code is loosened up to accept no clock/reset. This change is based >> on the discussion [1]. > (snip) > > Regards > Martin
On Thu, Apr 19, 2018 at 08:03:38PM +0900, Masahiro Yamada wrote: > Historically, the clocks and resets are handled on the glue layer > side instead of the DWC3 core. For simple cases, dwc3-of-simple.c > takes care of arbitrary number of clocks and resets. The DT node > structure typically looks like as follows: > > dwc3-glue { > compatible = "foo,dwc3"; > clocks = ...; > resets = ...; > ... > > dwc3 { > compatible = "snps,dwc3"; > ... > }; > } > > By supporting the clocks and the reset in the dwc3/core.c, it will > be turned into a single node: > > dwc3 { > compatible = "foo,dwc3", "snps,dwc3"; > clocks = ...; > resets = ...; > ... > } > > This commit adds the binding of clocks and resets specific to this IP. > The number of clocks should generally be the same across SoCs, it is > just some SoCs either tie clocks together or do not provide software > control of some of the clocks. > > I took the clock names from the Synopsys datasheet: "ref" (ref_clk), > "bus_early" (bus_clk_early), and "suspend" (suspend_clk). > > I found only one reset line in the datasheet, hence the reset-names > property is omitted. > > Supporting those clocks and resets is the requirement for new platforms. > Enforcing the new binding breaks existing platforms since they specify > clocks and resets in their glue layer node, but nothing in the core > node. I listed such exceptional cases in the DT binding. The driver > code is loosened up to accept no clock/reset. This change is based > on the discussion [1]. > > I inserted reset_control_deassert() and clk_bulk_enable() before the > first register access, i.e. dwc3_cache_hwparams(). > > [1] https://patchwork.kernel.org/patch/10284265/ > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > Changes in v2: > - Make clocks specific to this IP based on Synopsys datasheet > - Use clk_bulk API > - Add description to struct header > > Documentation/devicetree/bindings/usb/dwc3.txt | 21 ++++++ > drivers/usb/dwc3/core.c | 89 +++++++++++++++++++++++++- > drivers/usb/dwc3/core.h | 8 +++ > 3 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt > index 0dbd308..feb1cc33 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -7,6 +7,27 @@ Required properties: > - compatible: must be "snps,dwc3" > - reg : Address and length of the register set for the device > - interrupts: Interrupts used by the dwc3 controller. > + - clock-names: should contain "ref", "bus_early", "suspend" > + - clocks: list of phandle and clock specifier pairs corresponding to > + entries in the clock-names property. > + - resets: a single pair of phandle and reset specifier This should be optional as some SoCs don't have separate, s/w controlled resets of modules. Otherise, for the DT binding: Reviewed-by: Rob Herring <robh@kernel.org> Rob -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, 2018-04-26 0:21 GMT+09:00 Rob Herring <robh@kernel.org>: >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 0dbd308..feb1cc33 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -7,6 +7,27 @@ Required properties: >> - compatible: must be "snps,dwc3" >> - reg : Address and length of the register set for the device >> - interrupts: Interrupts used by the dwc3 controller. >> + - clock-names: should contain "ref", "bus_early", "suspend" >> + - clocks: list of phandle and clock specifier pairs corresponding to >> + entries in the clock-names property. >> + - resets: a single pair of phandle and reset specifier > > This should be optional as some SoCs don't have separate, s/w controlled > resets of modules. OK. I will move resets to optional property. Please let ask a question. The number of clocks should be the same across SoCs. (Even if there is no s/w control for clocks, we should input something such as clk-fixed-rate.) On the other hand, the number of resets can be different across SoCs. If there is no s/w control for resets, we can make it optional. (optional = 1 or 0 reset) Is this what you mean? If we had something like reset-nop (or reset-dummy) in case no s/w control, we would be able to input something. I am not sure if this is the right thing, though. > Otherise, for the DT binding: > > Reviewed-by: Rob Herring <robh@kernel.org> >
On Fri, Apr 27, 2018 at 11:20 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Rob, > > > 2018-04-26 0:21 GMT+09:00 Rob Herring <robh@kernel.org>: > >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >>> index 0dbd308..feb1cc33 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -7,6 +7,27 @@ Required properties: >>> - compatible: must be "snps,dwc3" >>> - reg : Address and length of the register set for the device >>> - interrupts: Interrupts used by the dwc3 controller. >>> + - clock-names: should contain "ref", "bus_early", "suspend" >>> + - clocks: list of phandle and clock specifier pairs corresponding to >>> + entries in the clock-names property. >>> + - resets: a single pair of phandle and reset specifier >> >> This should be optional as some SoCs don't have separate, s/w controlled >> resets of modules. > > OK. I will move resets to optional property. > > > Please let ask a question. > > > The number of clocks should be the same across SoCs. > (Even if there is no s/w control for clocks, > we should input something such as clk-fixed-rate.) I guess if there's really not s/w control, then the number may vary, but let's worry about that when someone has that problem. > On the other hand, the number of resets can be different > across SoCs. If there is no s/w control for resets, > we can make it optional. (optional = 1 or 0 reset) > > Is this what you mean? Yes. I think it's just more common to not have a reset than not have clocks. > If we had something like reset-nop (or reset-dummy) > in case no s/w control, we would be able to input something. > I am not sure if this is the right thing, though. I don't think we should require things if they are not needed. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Martin, 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl <martin.blumenstingl@googlemail.com>: > Hello, > > On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Historically, the clocks and resets are handled on the glue layer >> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c >> takes care of arbitrary number of clocks and resets. The DT node >> structure typically looks like as follows: >> >> dwc3-glue { >> compatible = "foo,dwc3"; >> clocks = ...; >> resets = ...; >> ... >> >> dwc3 { >> compatible = "snps,dwc3"; >> ... >> }; >> } >> >> By supporting the clocks and the reset in the dwc3/core.c, it will >> be turned into a single node: >> >> dwc3 { >> compatible = "foo,dwc3", "snps,dwc3"; >> clocks = ...; >> resets = ...; >> ... >> } >> >> This commit adds the binding of clocks and resets specific to this IP. >> The number of clocks should generally be the same across SoCs, it is >> just some SoCs either tie clocks together or do not provide software >> control of some of the clocks. >> >> I took the clock names from the Synopsys datasheet: "ref" (ref_clk), >> "bus_early" (bus_clk_early), and "suspend" (suspend_clk). > looking at the code: this could mean that dwc3-exynos.c can be removed > mid-term (assuming the PHY and regulator handling can be > moved/removed/changed) > > does the datasheet state anything about the clock speeds? from > Documentation/devicetree/bindings/usb/dwc3-xilinx.txt: > "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation > and >= 60MHz for HS operation > >> I found only one reset line in the datasheet, hence the reset-names >> property is omitted. > does the datasheet state whether this is a level or a pulsed reset line? > on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared) > reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for > shared and pulsed reset lines") because the reset line is shared > between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...) > your current approach (having a vendor-specific "foo,dwc3" binding > along with the generic "snps,dwc3") would allow having > per-"of_device_id" settings which could indicate whether the reset > lines are level or pulsed reset if these are "implementation specific" Let me ask a question about your reset controller. (drivers/reset/reset-meson.c) All reset ID supports .reset, .assert, .deassert Is this correct? I believe you and I use the same DWC3 core IP. I suspect the difference is in the reset controller side. In my case, the reset line is asserted by default. (that is, all FFs in the RTL are put into the initial state on power-on) That's why only reset_deassert() will work for me, I think. What about your case? Is the reset line in deassert state on power-on? Then, the reset must be explicitly pulsed to put FFs into the initial state. Is this correct?
(adding Yixun from Amlogic to this mail) On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Martin, > > > 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl > <martin.blumenstingl@googlemail.com>: >> Hello, >> >> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada >> <yamada.masahiro@socionext.com> wrote: >>> Historically, the clocks and resets are handled on the glue layer >>> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c >>> takes care of arbitrary number of clocks and resets. The DT node >>> structure typically looks like as follows: >>> >>> dwc3-glue { >>> compatible = "foo,dwc3"; >>> clocks = ...; >>> resets = ...; >>> ... >>> >>> dwc3 { >>> compatible = "snps,dwc3"; >>> ... >>> }; >>> } >>> >>> By supporting the clocks and the reset in the dwc3/core.c, it will >>> be turned into a single node: >>> >>> dwc3 { >>> compatible = "foo,dwc3", "snps,dwc3"; >>> clocks = ...; >>> resets = ...; >>> ... >>> } >>> >>> This commit adds the binding of clocks and resets specific to this IP. >>> The number of clocks should generally be the same across SoCs, it is >>> just some SoCs either tie clocks together or do not provide software >>> control of some of the clocks. >>> >>> I took the clock names from the Synopsys datasheet: "ref" (ref_clk), >>> "bus_early" (bus_clk_early), and "suspend" (suspend_clk). >> looking at the code: this could mean that dwc3-exynos.c can be removed >> mid-term (assuming the PHY and regulator handling can be >> moved/removed/changed) >> >> does the datasheet state anything about the clock speeds? from >> Documentation/devicetree/bindings/usb/dwc3-xilinx.txt: >> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation >> and >= 60MHz for HS operation >> >>> I found only one reset line in the datasheet, hence the reset-names >>> property is omitted. >> does the datasheet state whether this is a level or a pulsed reset line? >> on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared) >> reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for >> shared and pulsed reset lines") because the reset line is shared >> between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...) >> your current approach (having a vendor-specific "foo,dwc3" binding >> along with the generic "snps,dwc3") would allow having >> per-"of_device_id" settings which could indicate whether the reset >> lines are level or pulsed reset if these are "implementation specific" > > Let me ask a question about your reset controller. > (drivers/reset/reset-meson.c) > > All reset ID supports .reset, .assert, .deassert > Is this correct? as far as I know: yes (though I have only ever verified this with the Ethernet controller's reset line) > > I believe you and I use the same DWC3 core IP. this is possible - but I am not sure since I don't have access to Amlogic's internal resources where this should be documented (my knowledge mostly comes from reading Amlogic's out-of-tree kernel code and porting that to mainline) > > I suspect the difference is in the reset controller side. > > In my case, the reset line is asserted by default. > (that is, all FFs in the RTL are put into the initial state > on power-on) > That's why only reset_deassert() will work for me, I think. > > What about your case? Is the reset line in deassert state on power-on? > Then, the reset must be explicitly pulsed to put FFs into > the initial state. Is this correct? let me give you a bit of context first: the Amlogic Meson AXG, GXL and GXM SoCs have one reset line for "USB components". this is shared among: - the dwc3 controller - (depending on the SoC) 2 or 3 USB2 PHYs - a USB3 PHY - some OTG detection logic within the registers of the USB3 PHY (there is also a gate clock which is assigned to the same components) based on my tests I believe that the reset line is "de-asserted" (= USB components are working) by default. asserting that reset line should stop the state machine of all USB components. de-asserting it again should bring all USB components into a defined state. (I'm not sure though if these are HW defaults or if there's some logic in the bootrom / early stage [pre u-boot] bootloaders) that said, the "reset" framework currently cannot handle level resets with shared reset lines which are de-asserted by default. to bring the USB components into a defined state I would have to use reset_control_assert() first, then reset_control_deassert(). the reset framework reports an error in this case: [0] using a reset pulse however works in any case, the reset framework ensures that it's only executed once for all shared reset lines (our reset controller hardware probably asserts and de-asserts the reset line internally - this is just speculation though) Regards Martin [0] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/reset/core.c#L317 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Martin, 2018-04-28 23:20 GMT+09:00 Martin Blumenstingl <martin.blumenstingl@googlemail.com>: > (adding Yixun from Amlogic to this mail) > > On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> Hi Martin, >> >> >> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl >> <martin.blumenstingl@googlemail.com>: >>> Hello, >>> >>> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada >>> <yamada.masahiro@socionext.com> wrote: >>>> Historically, the clocks and resets are handled on the glue layer >>>> side instead of the DWC3 core. For simple cases, dwc3-of-simple.c >>>> takes care of arbitrary number of clocks and resets. The DT node >>>> structure typically looks like as follows: >>>> >>>> dwc3-glue { >>>> compatible = "foo,dwc3"; >>>> clocks = ...; >>>> resets = ...; >>>> ... >>>> >>>> dwc3 { >>>> compatible = "snps,dwc3"; >>>> ... >>>> }; >>>> } >>>> >>>> By supporting the clocks and the reset in the dwc3/core.c, it will >>>> be turned into a single node: >>>> >>>> dwc3 { >>>> compatible = "foo,dwc3", "snps,dwc3"; >>>> clocks = ...; >>>> resets = ...; >>>> ... >>>> } >>>> >>>> This commit adds the binding of clocks and resets specific to this IP. >>>> The number of clocks should generally be the same across SoCs, it is >>>> just some SoCs either tie clocks together or do not provide software >>>> control of some of the clocks. >>>> >>>> I took the clock names from the Synopsys datasheet: "ref" (ref_clk), >>>> "bus_early" (bus_clk_early), and "suspend" (suspend_clk). >>> looking at the code: this could mean that dwc3-exynos.c can be removed >>> mid-term (assuming the PHY and regulator handling can be >>> moved/removed/changed) >>> >>> does the datasheet state anything about the clock speeds? from >>> Documentation/devicetree/bindings/usb/dwc3-xilinx.txt: >>> "bus_clk" Master/Core clock, have to be >= 125 MHz for SS operation >>> and >= 60MHz for HS operation >>> >>>> I found only one reset line in the datasheet, hence the reset-names >>>> property is omitted. >>> does the datasheet state whether this is a level or a pulsed reset line? >>> on Amlogic Meson GXL, GXM and AXG SoCs we use a pulsed (and shared) >>> reset line (see ff0a632f08759e "usb: dwc3: of-simple: add support for >>> shared and pulsed reset lines") because the reset line is shared >>> between various components (USB2 PHY, USB3 PHY, dwc3 controller, ...) >>> your current approach (having a vendor-specific "foo,dwc3" binding >>> along with the generic "snps,dwc3") would allow having >>> per-"of_device_id" settings which could indicate whether the reset >>> lines are level or pulsed reset if these are "implementation specific" >> >> Let me ask a question about your reset controller. >> (drivers/reset/reset-meson.c) >> >> All reset ID supports .reset, .assert, .deassert >> Is this correct? > as far as I know: yes (though I have only ever verified this with the > Ethernet controller's reset line) > >> >> I believe you and I use the same DWC3 core IP. > this is possible - but I am not sure since I don't have access to > Amlogic's internal resources where this should be documented (my > knowledge mostly comes from reading Amlogic's out-of-tree kernel code > and porting that to mainline) > >> >> I suspect the difference is in the reset controller side. >> >> In my case, the reset line is asserted by default. >> (that is, all FFs in the RTL are put into the initial state >> on power-on) >> That's why only reset_deassert() will work for me, I think. >> >> What about your case? Is the reset line in deassert state on power-on? >> Then, the reset must be explicitly pulsed to put FFs into >> the initial state. Is this correct? > let me give you a bit of context first: > the Amlogic Meson AXG, GXL and GXM SoCs have one reset line for "USB > components". this is shared among: > - the dwc3 controller > - (depending on the SoC) 2 or 3 USB2 PHYs > - a USB3 PHY > - some OTG detection logic within the registers of the USB3 PHY > > (there is also a gate clock which is assigned to the same components) > > based on my tests I believe that the reset line is "de-asserted" (= > USB components are working) by default. > asserting that reset line should stop the state machine of all USB > components. de-asserting it again should bring all USB components into > a defined state. > (I'm not sure though if these are HW defaults or if there's some logic > in the bootrom / early stage [pre u-boot] bootloaders) > > that said, the "reset" framework currently cannot handle level resets > with shared reset lines which are de-asserted by default. > to bring the USB components into a defined state I would have to use > reset_control_assert() first, then reset_control_deassert(). the reset > framework reports an error in this case: [0] > using a reset pulse however works in any case, the reset framework > ensures that it's only executed once for all shared reset lines (our > reset controller hardware probably asserts and de-asserts the reset > line internally - this is just speculation though) > > > Regards > Martin > > > [0] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/reset/core.c#L317 Sorry for the late reply. Personally, I'd like to see a generic solution instead of tweaking the reset consumer (dwc3-of-simple.c) I am not sure what the right thing to do, but just threw this post: https://lkml.org/lkml/2018/5/10/116
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 0dbd308..feb1cc33 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -7,6 +7,27 @@ Required properties: - compatible: must be "snps,dwc3" - reg : Address and length of the register set for the device - interrupts: Interrupts used by the dwc3 controller. + - clock-names: should contain "ref", "bus_early", "suspend" + - clocks: list of phandle and clock specifier pairs corresponding to + entries in the clock-names property. + - resets: a single pair of phandle and reset specifier + +Exception for clocks and resets: + clocks and resets are optional if the parent node (i.e. glue-layer) + is compatible to one of the following: + "amlogic,meson-axg-dwc3" + "amlogic,meson-gxl-dwc3" + "cavium,octeon-7130-usb-uctl" + "qcom,dwc3" + "samsung,exynos5250-dwusb3" + "samsung,exynos7-dwusb3" + "sprd,sc9860-dwc3" + "st,stih407-dwc3" + "ti,am437x-dwc3" + "ti,dwc3" + "ti,keystone-dwc3" + "rockchip,rk3399-dwc3" + "xlnx,zynqmp-dwc3" Optional properties: - usb-phy : array of phandle for the PHY device. The first element diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 8e66edd..15e1613 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -8,6 +8,7 @@ * Sebastian Andrzej Siewior <bigeasy@linutronix.de> */ +#include <linux/clk.h> #include <linux/version.h> #include <linux/module.h> #include <linux/kernel.h> @@ -24,6 +25,7 @@ #include <linux/of.h> #include <linux/acpi.h> #include <linux/pinctrl/consumer.h> +#include <linux/reset.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> @@ -266,6 +268,12 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) return 0; } +static const struct clk_bulk_data dwc3_core_clks[] = { + { .id = "ref" }, + { .id = "bus_early" }, + { .id = "suspend" }, +}; + /* * dwc3_frame_length_adjustment - Adjusts frame length if required * @dwc3: Pointer to our controller context structure @@ -667,6 +675,9 @@ static void dwc3_core_exit(struct dwc3 *dwc) usb_phy_set_suspend(dwc->usb3_phy, 1); phy_power_off(dwc->usb2_generic_phy); phy_power_off(dwc->usb3_generic_phy); + clk_bulk_disable(dwc->num_clks, dwc->clks); + clk_bulk_unprepare(dwc->num_clks, dwc->clks); + reset_control_assert(dwc->reset); } static bool dwc3_core_is_valid(struct dwc3 *dwc) @@ -1256,6 +1267,12 @@ static int dwc3_probe(struct platform_device *pdev) if (!dwc) return -ENOMEM; + dwc->clks = devm_kmemdup(dev, dwc3_core_clks, sizeof(dwc3_core_clks), + GFP_KERNEL); + if (!dwc->clks) + return -ENOMEM; + + dwc->num_clks = ARRAY_SIZE(dwc3_core_clks); dwc->dev = dev; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1286,6 +1303,33 @@ static int dwc3_probe(struct platform_device *pdev) dwc3_get_properties(dwc); + /* Reset is optional to save existing platforms. */ + dwc->reset = devm_reset_control_get_optional_shared(dev, NULL); + if (IS_ERR(dwc->reset)) + return PTR_ERR(dwc->reset); + + ret = clk_bulk_get(dev, dwc->num_clks, dwc->clks); + if (ret == -EPROBE_DEFER) + return ret; + /* + * Clocks are optional to save existing platforms. New platforms should + * support all clocks as required by the DT-binding. + */ + if (ret) + dwc->num_clks = 0; + + ret = reset_control_deassert(dwc->reset); + if (ret) + goto put_clks; + + ret = clk_bulk_prepare(dwc->num_clks, dwc->clks); + if (ret) + goto assert_reset; + + ret = clk_bulk_enable(dwc->num_clks, dwc->clks); + if (ret) + goto unprepare_clks; + platform_set_drvdata(pdev, dwc); dwc3_cache_hwparams(dwc); @@ -1349,6 +1393,14 @@ static int dwc3_probe(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); + clk_bulk_disable(dwc->num_clks, dwc->clks); +unprepare_clks: + clk_bulk_unprepare(dwc->num_clks, dwc->clks); +assert_reset: + reset_control_assert(dwc->reset); +put_clks: + clk_bulk_put(dwc->num_clks, dwc->clks); + return ret; } @@ -1370,11 +1422,44 @@ static int dwc3_remove(struct platform_device *pdev) dwc3_free_event_buffers(dwc); dwc3_free_scratch_buffers(dwc); + clk_bulk_put(dwc->num_clks, dwc->clks); return 0; } #ifdef CONFIG_PM +static int dwc3_core_init_for_resume(struct dwc3 *dwc) +{ + int ret; + + ret = reset_control_deassert(dwc->reset); + if (ret) + return ret; + + ret = clk_bulk_prepare(dwc->num_clks, dwc->clks); + if (ret) + goto assert_reset; + + ret = clk_bulk_enable(dwc->num_clks, dwc->clks); + if (ret) + goto unprepare_clks; + + ret = dwc3_core_init(dwc); + if (ret) + goto disable_clks; + + return 0; + +disable_clks: + clk_bulk_disable(dwc->num_clks, dwc->clks); +unprepare_clks: + clk_bulk_unprepare(dwc->num_clks, dwc->clks); +assert_reset: + reset_control_assert(dwc->reset); + + return ret; +} + static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) { unsigned long flags; @@ -1420,7 +1505,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: - ret = dwc3_core_init(dwc); + ret = dwc3_core_init_for_resume(dwc); if (ret) return ret; @@ -1432,7 +1517,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) case DWC3_GCTL_PRTCAP_HOST: /* nothing to do on host runtime_resume */ if (!PMSG_IS_AUTO(msg)) { - ret = dwc3_core_init(dwc); + ret = dwc3_core_init_for_resume(dwc); if (ret) return ret; dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 4f3b438..1765e01 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -891,6 +891,9 @@ struct dwc3_scratchpad_array { * @eps: endpoint array * @gadget: device side representation of the peripheral controller * @gadget_driver: pointer to the gadget driver + * @clks: array of clocks + * @num_clks: number of clocks + * @reset: reset control * @regs: base address for our registers * @regs_size: address space size * @fladj: frame length adjustment @@ -1013,6 +1016,11 @@ struct dwc3 { struct usb_gadget gadget; struct usb_gadget_driver *gadget_driver; + struct clk_bulk_data *clks; + int num_clks; + + struct reset_control *reset; + struct usb_phy *usb2_phy; struct usb_phy *usb3_phy;
Historically, the clocks and resets are handled on the glue layer side instead of the DWC3 core. For simple cases, dwc3-of-simple.c takes care of arbitrary number of clocks and resets. The DT node structure typically looks like as follows: dwc3-glue { compatible = "foo,dwc3"; clocks = ...; resets = ...; ... dwc3 { compatible = "snps,dwc3"; ... }; } By supporting the clocks and the reset in the dwc3/core.c, it will be turned into a single node: dwc3 { compatible = "foo,dwc3", "snps,dwc3"; clocks = ...; resets = ...; ... } This commit adds the binding of clocks and resets specific to this IP. The number of clocks should generally be the same across SoCs, it is just some SoCs either tie clocks together or do not provide software control of some of the clocks. I took the clock names from the Synopsys datasheet: "ref" (ref_clk), "bus_early" (bus_clk_early), and "suspend" (suspend_clk). I found only one reset line in the datasheet, hence the reset-names property is omitted. Supporting those clocks and resets is the requirement for new platforms. Enforcing the new binding breaks existing platforms since they specify clocks and resets in their glue layer node, but nothing in the core node. I listed such exceptional cases in the DT binding. The driver code is loosened up to accept no clock/reset. This change is based on the discussion [1]. I inserted reset_control_deassert() and clk_bulk_enable() before the first register access, i.e. dwc3_cache_hwparams(). [1] https://patchwork.kernel.org/patch/10284265/ Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Changes in v2: - Make clocks specific to this IP based on Synopsys datasheet - Use clk_bulk API - Add description to struct header Documentation/devicetree/bindings/usb/dwc3.txt | 21 ++++++ drivers/usb/dwc3/core.c | 89 +++++++++++++++++++++++++- drivers/usb/dwc3/core.h | 8 +++ 3 files changed, 116 insertions(+), 2 deletions(-)