In the current design of DWC3 driver,
the DT typically becomes a nested structure like follows:
dwc3-glue {
compatible = "foo,dwc3";
...
dwc3 {
compatible = "snps,dwc3";
...
};
}
The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
clocks / resets at all.
The only solution we have now, is to put DWC3 core node under
the glue layer node, then add clocks and resets there.
Actually, dwc3-of-simple.c exists to handle clocks and resets.
As always for digital circuits, DWC3 core IP itself needs clock input.
This is specific to this IP. So, supporting clocks and resets in
dwc3/core.c makes sense.
In this version, the number of clocks (and names) is specific
to this IP, with clock names taken from Synopsys datasheet.
Masahiro Yamada (2):
usb: dwc3: use local copy of resource to fix-up register offset
usb: dwc3: support clocks and resets for DWC3 core
Documentation/devicetree/bindings/usb/dwc3.txt | 21 +++++
drivers/usb/dwc3/core.c | 119 +++++++++++++++++++------
drivers/usb/dwc3/core.h | 8 ++
3 files changed, 123 insertions(+), 25 deletions(-)
--
2.7.4
It is not a good idea to directly modify the resource of a platform
device. Modify its local copy, and pass it to devm_ioremap_resource()
so that we do not need to restore it in the failure path and the remove
hook.
Signed-off-by: Masahiro Yamada <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
---
Changes in v2: None
drivers/usb/dwc3/core.c | 32 ++++++++------------------------
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a15648d..8e66edd 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1245,7 +1245,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct resource *res;
+ struct resource *res, dwc_res;
struct dwc3 *dwc;
int ret;
@@ -1270,20 +1270,19 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->xhci_resources[0].flags = res->flags;
dwc->xhci_resources[0].name = res->name;
- res->start += DWC3_GLOBALS_REGS_START;
-
/*
* Request memory region but exclude xHCI regs,
* since it will be requested by the xhci-plat driver.
*/
- regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(regs)) {
- ret = PTR_ERR(regs);
- goto err0;
- }
+ dwc_res = *res;
+ dwc_res.start += DWC3_GLOBALS_REGS_START;
+
+ regs = devm_ioremap_resource(dev, &dwc_res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
dwc->regs = regs;
- dwc->regs_size = resource_size(res);
+ dwc->regs_size = resource_size(&dwc_res);
dwc3_get_properties(dwc);
@@ -1350,29 +1349,14 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err0:
- /*
- * restore res->start back to its original value so that, in case the
- * probe is deferred, we don't end up getting error in request the
- * memory region the next time probe is called.
- */
- res->start -= DWC3_GLOBALS_REGS_START;
-
return ret;
}
static int dwc3_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pm_runtime_get_sync(&pdev->dev);
- /*
- * restore res->start back to its original value so that, in case the
- * probe is deferred, we don't end up getting error in request the
- * memory region the next time probe is called.
- */
- res->start -= DWC3_GLOBALS_REGS_START;
dwc3_debugfs_exit(dwc);
dwc3_core_exit_mode(dwc);
--
2.7.4
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 <[email protected]>
---
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 <[email protected]>
*/
+#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;
--
2.7.4
Hello,
On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
<[email protected]> 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
HI,
On 4/19/2018 4:03 AM, Masahiro Yamada wrote:
> In the current design of DWC3 driver,
> the DT typically becomes a nested structure like follows:
>
> dwc3-glue {
> compatible = "foo,dwc3";
> ...
>
> dwc3 {
> compatible = "snps,dwc3";
> ...
> };
> }
>
> The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
> clocks / resets at all.
>
> The only solution we have now, is to put DWC3 core node under
> the glue layer node, then add clocks and resets there.
> Actually, dwc3-of-simple.c exists to handle clocks and resets.
>
> As always for digital circuits, DWC3 core IP itself needs clock input.
> This is specific to this IP. So, supporting clocks and resets in
> dwc3/core.c makes sense.
Why can't dwc3-of-simple be used with this IP?
Adding core/reset handling in both core and glue drivers might
only add to confusion and I cant think of a reason why having a parent
node representing dwc3-of-simple glue would be any concern.
Or are you planning to remove dwc3-of-simple.c driver?
>
> In this version, the number of clocks (and names) is specific
> to this IP, with clock names taken from Synopsys datasheet.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
<[email protected]>:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
> <[email protected]> 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
2018-04-24 9:11 GMT+09:00 Manu Gautam <[email protected]>:
> HI,
>
>
> On 4/19/2018 4:03 AM, Masahiro Yamada wrote:
>> In the current design of DWC3 driver,
>> the DT typically becomes a nested structure like follows:
>>
>> dwc3-glue {
>> compatible = "foo,dwc3";
>> ...
>>
>> dwc3 {
>> compatible = "snps,dwc3";
>> ...
>> };
>> }
>>
>> The current DWC3 core (drivers/usb/dwc3/core.c) can not handle
>> clocks / resets at all.
>>
>> The only solution we have now, is to put DWC3 core node under
>> the glue layer node, then add clocks and resets there.
>> Actually, dwc3-of-simple.c exists to handle clocks and resets.
>>
>> As always for digital circuits, DWC3 core IP itself needs clock input.
>> This is specific to this IP. So, supporting clocks and resets in
>> dwc3/core.c makes sense.
>
> Why can't dwc3-of-simple be used with this IP?
> Adding core/reset handling in both core and glue drivers might
> only add to confusion and I cant think of a reason why having a parent
> node representing dwc3-of-simple glue would be any concern.
> Or are you planning to remove dwc3-of-simple.c driver?
dwc3-of-simple.c can be removed only after at least
all upstream DT files are migrated.
(and give enough time for migration of downstream DT)
At least, new platforms are not required to use this hack.
--
Best Regards
Masahiro Yamada
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 <[email protected]>
> ---
>
> 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 <[email protected]>
Rob
Hi Rob,
2018-04-26 0:21 GMT+09:00 Rob Herring <[email protected]>:
>> 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 <[email protected]>
>
--
Best Regards
Masahiro Yamada
On Fri, Apr 27, 2018 at 11:20 AM, Masahiro Yamada
<[email protected]> wrote:
> Hi Rob,
>
>
> 2018-04-26 0:21 GMT+09:00 Rob Herring <[email protected]>:
>
>>> 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
Hi Martin,
2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
<[email protected]>:
> Hello,
>
> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
> <[email protected]> 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
(adding Yixun from Amlogic to this mail)
On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada
<[email protected]> wrote:
> Hi Martin,
>
>
> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
> <[email protected]>:
>> Hello,
>>
>> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
>> <[email protected]> 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
Hi Felipe,
2018-04-19 20:03 GMT+09:00 Masahiro Yamada <[email protected]>:
> It is not a good idea to directly modify the resource of a platform
> device. Modify its local copy, and pass it to devm_ioremap_resource()
> so that we do not need to restore it in the failure path and the remove
> hook.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> Reviewed-by: Masami Hiramatsu <[email protected]>
I want this patch applied first
unless you are opposed to this clean-up.
I'd like to avoid re-sending a trivial patch like this.
> ---
>
> Changes in v2: None
>
> drivers/usb/dwc3/core.c | 32 ++++++++------------------------
> 1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a15648d..8e66edd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1245,7 +1245,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
> static int dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> - struct resource *res;
> + struct resource *res, dwc_res;
> struct dwc3 *dwc;
>
> int ret;
> @@ -1270,20 +1270,19 @@ static int dwc3_probe(struct platform_device *pdev)
> dwc->xhci_resources[0].flags = res->flags;
> dwc->xhci_resources[0].name = res->name;
>
> - res->start += DWC3_GLOBALS_REGS_START;
> -
> /*
> * Request memory region but exclude xHCI regs,
> * since it will be requested by the xhci-plat driver.
> */
> - regs = devm_ioremap_resource(dev, res);
> - if (IS_ERR(regs)) {
> - ret = PTR_ERR(regs);
> - goto err0;
> - }
> + dwc_res = *res;
> + dwc_res.start += DWC3_GLOBALS_REGS_START;
> +
> + regs = devm_ioremap_resource(dev, &dwc_res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
>
> dwc->regs = regs;
> - dwc->regs_size = resource_size(res);
> + dwc->regs_size = resource_size(&dwc_res);
>
> dwc3_get_properties(dwc);
>
> @@ -1350,29 +1349,14 @@ static int dwc3_probe(struct platform_device *pdev)
> pm_runtime_put_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
>
> -err0:
> - /*
> - * restore res->start back to its original value so that, in case the
> - * probe is deferred, we don't end up getting error in request the
> - * memory region the next time probe is called.
> - */
> - res->start -= DWC3_GLOBALS_REGS_START;
> -
> return ret;
> }
>
> static int dwc3_remove(struct platform_device *pdev)
> {
> struct dwc3 *dwc = platform_get_drvdata(pdev);
> - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> pm_runtime_get_sync(&pdev->dev);
> - /*
> - * restore res->start back to its original value so that, in case the
> - * probe is deferred, we don't end up getting error in request the
> - * memory region the next time probe is called.
> - */
> - res->start -= DWC3_GLOBALS_REGS_START;
>
> dwc3_debugfs_exit(dwc);
> dwc3_core_exit_mode(dwc);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
Hi Martin,
2018-04-28 23:20 GMT+09:00 Martin Blumenstingl
<[email protected]>:
> (adding Yixun from Amlogic to this mail)
>
> On Sat, Apr 28, 2018 at 4:41 AM, Masahiro Yamada
> <[email protected]> wrote:
>> Hi Martin,
>>
>>
>> 2018-04-24 2:44 GMT+09:00 Martin Blumenstingl
>> <[email protected]>:
>>> Hello,
>>>
>>> On Thu, Apr 19, 2018 at 1:03 PM, Masahiro Yamada
>>> <[email protected]> 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
--
Best Regards
Masahiro Yamada