Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Masahiro Yamada]

Hi Martin,


2018-04-28 23:20 GMT+09:00 Martin Blumenstingl
:
> (adding Yixun from Amlogic to this mail)
>
> On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada
>  wrote:
>> Hi Martin,
>>
>>
>> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
>> :
>>> Hello,
>>>
>>> On Thu, Apr 19, 2018 at 1:03 PM, 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).
>>> 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
> 

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Martin Blumenstingl]

(adding Yixun from Amlogic to this mail)

On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada
 wrote:
> Hi Martin,
>
>
> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
> :
>> Hello,
>>
>> On Thu, Apr 19, 2018 at 1:03 PM, 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).
>> 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] 

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Masahiro Yamada]

Hi Martin,


2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, 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).
> 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?




-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Rob Herring]

On Fri, Apr 27, 2018 at 11:20 AM, Masahiro Yamada
 wrote:
> Hi Rob,
>
>
> 2018-04-26 0:21 GMT+09:00 Rob Herring :
>
>>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Masahiro Yamada]

Hi Rob,


2018-04-26 0:21 GMT+09:00 Rob Herring :

>> 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 
>


-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Rob Herring]

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 
> ---
> 
> 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 

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Masahiro Yamada]

2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, 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).
> 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



-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Martin Blumenstingl]

Hello,

On Thu, Apr 19, 2018 at 1:03 PM, 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).
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] usb: dwc3: support clocks and resets for DWC3 core

[Masahiro Yamada]

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 
---

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
+
+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 
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -24,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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