Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

From: Felipe Balbi
Date: Mon Dec 29 2014 - 11:15:48 EST

Hi,

On Mon, Dec 29, 2014 at 01:52:04AM +0800, Sneeker Yeh wrote:
> Hi,
>
> 2014-12-22 23:59 GMT+08:00 Felipe Balbi <balbi@xxxxxx>:
>
> > Hi,
> >
> > On Tue, Dec 16, 2014 at 10:10:27AM +0800, Sneeker Yeh wrote:
> > > This patch adds support for Synopsis DesignWare USB3 IP Core found
> > > on Fujitsu Socs.
> > >
> > > Signed-off-by: Sneeker Yeh <Sneeker.Yeh@xxxxxxxxxxxxxx>
> > > ---
> > > .../devicetree/bindings/usb/fujitsu-dwc3.txt | 25 +++
> > > drivers/usb/dwc3/Kconfig | 11 ++
> > > drivers/usb/dwc3/Makefile | 1 +
> > > drivers/usb/dwc3/dwc3-mb86s70.c | 194
> > ++++++++++++++++++++
> > > 4 files changed, 231 insertions(+)
> > > create mode 100644
> > Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > new file mode 100644
> > > index 0000000..df77f91
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > @@ -0,0 +1,25 @@
> > > +FUJITSU GLUE COMPONENTS
> > > +
> > > +MB86S7x DWC3 GLUE
> > > + - compatible : Should be "fujitsu,mb86s70-dwc3"
> > > + - clocks: from common clock binding, handle to usb clock.
> > > + - clock-names: from common clock binding.
> > > + - #address-cells, #size-cells : Must be present if the device has
> > sub-nodes
> > > + - ranges: the child address space are mapped 1:1 onto the parent
> > address space
> > > + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
> > > +
> > > +Sub-nodes:
> > > +The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
> > > +- dwc3 :
> > > + The binding details of dwc3 can be found in:
> > > + Documentation/devicetree/bindings/usb/dwc3.txt
> > > +
> > > +usb3host: mb86s70_usb3host {
> > > + compatible = "fujitsu,mb86s70-dwc3";
> > > + clocks = <&clk_alw_1_1>;
> > > + clock-names = "bus_clk";
> > > + #address-cells = <2>;
> > > + #size-cells = <1>;
> > > + ranges;
> > > + #stream-id-cells = <0>;
> > > +};
> > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > > index 58b5b2c..3390d42 100644
> > > --- a/drivers/usb/dwc3/Kconfig
> > > +++ b/drivers/usb/dwc3/Kconfig
> > > @@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
> > > Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
> > > say 'Y' or 'M' if you have one such device.
> > >
> > > +config USB_DWC3_MB86S70
> > > + tristate "MB86S70 Designware USB3 Platform code"
> > > + default USB_DWC3
> > > + help
> > > + MB86S7X SOC ship with DesignWare Core USB3 IP inside,
> > > + this implementation also integrated Fujitsu USB PHY inside
> > > + this Core USB3 IP.
> > > +
> > > + say 'Y' or 'M' if you have one such device.
> > > +
> > > +
> > > config USB_DWC3_PCI
> > > tristate "PCIe-based Platforms"
> > > depends on PCI
> > > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > > index bb34fbc..05d1de2 100644
> > > --- a/drivers/usb/dwc3/Makefile
> > > +++ b/drivers/usb/dwc3/Makefile
> > > @@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o
> > > obj-$(CONFIG_USB_DWC3_KEYSTONE) += dwc3-keystone.o
> > > obj-$(CONFIG_USB_DWC3_QCOM) += dwc3-qcom.o
> > > obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o
> > > +obj-$(CONFIG_USB_DWC3_MB86S70) += dwc3-mb86s70.o
> > > diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c
> > b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > new file mode 100644
> > > index 0000000..241c5bd
> > > --- /dev/null
> > > +++ b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > @@ -0,0 +1,194 @@
> > > +/**
> > > + * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
> > > + *
> > > + * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
> > > + * http://jp.fujitsu.com/group/fsl
> > > + *
> > > + * Author: Alice Chan <Alice.Chan@xxxxxxxxxxxxxx>
> > > + * based on dwc3-exynos.c
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/clk.h>
> > > +
> > > +struct dwc3_mb86s70 {
> > > + struct device *dev;
> > > + struct clk **clk_table;
> >
> > please align these.
>
>
> okay.
>
>
> >
>
>
> > > +};
> > > +
> > > +/* return 0 means successful */
> >
> > this comment is unnecessary :-)
> >
>
> okay
>
>
> >
> > > +static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
> > > +{
> > > + int ret, i;
> > > + struct clk *clk;
> > > +
> > > + if (!on)
> > > + goto clock_off;
> > > +
> > > + for (i = 0;; i++) {
> > > + clk = of_clk_get(dev->of_node, i);
> >
> > you could count the number of properties first, then allocate clk_table
> > to the exact size.
> >
>
> okay, thanks.
>
>
> >
> > > + if (IS_ERR(clk))
> > > + break;
> > > +
> > > + ret = clk_prepare_enable(clk);
> > > + if (ret) {
> > > + dev_err(dev, "failed to enable clock[%d]\n", i);
> > > + goto clock_off;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +clock_off:
> > > + for (i = 0;; i++) {
> > > + clk = of_clk_get(dev->of_node, i);
> > > + if (IS_ERR(clk))
> > > + break;
> > > +
> > > + clk_disable_unprepare(clk);
> > > + }
> > > +
> > > + return on;
> > > +}
> > > +
> > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dev);
> > > +
> > > + of_device_unregister(pdev);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> >
> > why ? Use dma_coerce_mask_and_coherent().
> >
>
> okay.
>
>
> >
> > > +
> > > +static int dwc3_mb86s70_probe(struct platform_device *pdev)
> > > +{
> > > + struct dwc3_mb86s70 *mb86s70;
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *node = dev->of_node;
> > > + int ret;
> > > +
> > > + mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
> > > + if (!mb86s70) {
> > > + ret = -ENOMEM;
> > > + goto err;
> > > + }
> > > +
> > > + dev->dma_mask = &dwc3_mb86s70_dma_mask;
> > > +
> > > + platform_set_drvdata(pdev, mb86s70);
> > > +
> > > + mb86s70->dev = dev;
> > > +
> > > + /* resume driver for clock, power */
> > > + pm_runtime_enable(&pdev->dev);
> > > + ret = pm_runtime_get_sync(&pdev->dev);
> >
> > here's a slightly better way (add error checking where necessary):
> >
> > dwc3_mb86s70_clk_control(dev, true);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get(dev);
> >
> >
> okay, thanks,
> these seems much better and more correct.
>
>
>
> >
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "get_sync failed with err %d\n", ret);
> > > + goto err;
> >
> > if get_sync() fails, you still need to call pm_runtime_put_sync(); In
> > this case, you also need to call pm_runtime_disable();
> >
>
> okay, thanks.
>
>
> >
> > > + }
> > > +
> > > + if (node) {
> > > + ret = of_platform_populate(node, NULL, NULL, dev);
> > > + if (!ret)
> > > + return 0;
> > > + dev_err(dev, "failed to add dwc3 core\n");
> > > + }
> > > + dev_err(dev, "no device node, failed to add dwc3 core\n");
> > > + ret = -ENODEV;
> > > +
> > > +err:
> > > + return ret;
> > > +}
> > > +
> > > +static int dwc3_mb86s70_remove(struct platform_device *pdev)
> > > +{
> > > + device_for_each_child(&pdev->dev, NULL, dwc3_mb86s70_remove_child);
> > > +
> > > + pm_runtime_put_sync(&pdev->dev);
> > > + pm_runtime_disable(&pdev->dev);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id mb86s70_dwc3_match[] = {
> > > + { .compatible = "fujitsu,mb86s70-dwc3" },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
> > > +
> > > +#ifdef CONFIG_PM
> > > +#ifdef CONFIG_PM_RUNTIME
> > > +static int dwc3_mb86s70_runtime_suspend(struct device *dev)
> > > +{
> > > + dwc3_mb86s70_clk_control(dev, false);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int dwc3_mb86s70_runtime_resume(struct device *dev)
> > > +{
> > > + dwc3_mb86s70_clk_control(dev, true);
> > > +
> > > + return 0;
> > > +}
> > > +#endif /* CONFIG_PM_RUNTIME */
> > > +static int dwc3_mb86s70_suspend(struct device *dev)
> > > +{
> > > + if (pm_runtime_status_suspended(dev))
> > > + return 0;
> > > +
> > > + return dwc3_mb86s70_runtime_suspend(dev);
> >
> > ok, this is a bit confusing and, in fact, wrong. You're not dealing with
> > wakeups (which are mandatory on runtime_pm and option on system sleep).
> >
> > It's okay to have a common function called by both runtime_pm and
> > system_sleep methods, but calling your runtime_pm handler is a bit odd.
> >
>
> hm, sorry I might confuse people here.
> I'll change this to what you suggested.
>
>
> >
> > last but not least, I need to see results of this running on top of
> > v3.19-rc1. Are you using both host and peripheral modes or only one mode
> > for now ?
> >
>
> The dwc3 IP we adapted is host-only, and we run this platform glue driver
> in v3.19-rc1 for now.
> bellow log is about suspend and resume successfully, and dwc3 still works.
>
> root@linaro-nano:~# echo mem > /sys/power/state
> [ 56.713281] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
> started.
> [ 56.720721] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_suspend() is started.
> [ 56.729419] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_suspend() is ended.
> [ 56.737363] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
> ended.
> [ 56.744604] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
> started.
> [ 56.752022] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_suspend() is started.
> [ 56.760700] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_suspend() is ended.
> [ 56.768639] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
> ended.
> [ 57.107139] SUSPEND
>
> [ 74.564963] mmc0: execute_tuning --
> [ 74.877776] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> started.
> [ 74.885126] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_resume() is started.
> [ 74.893749] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_resume() is ended.
> [ 74.901606] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> ended.
> [ 74.908766] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> started.
> [ 74.916086] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_resume() is started.
> [ 74.924660] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_resume() is ended.
> [ 74.932513] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> ended.
> root@linaro-nano:~#
>
> But I realize something might confuse people, and even is wrong, through
> your comment.
> So I guess I should align these to a more clear form like dwc3-exynos.c :
>
> static int dwc3_exynos_suspend(struct device *dev)
> {
> struct dwc3_exynos *exynos = dev_get_drvdata(dev);
>
> clk_disable(exynos->clk);
>
> return 0;
> }
>
> static int dwc3_exynos_resume(struct device *dev)
> {
> struct dwc3_exynos *exynos = dev_get_drvdata(dev);
>
> clk_enable(exynos->clk);
>
> /* runtime set active to reflect active state. */
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
>
> return 0;
> }
>
> Would it been better than how I do suspend/resume here?

sure, that would be better :-)

> however recently I got some thought inspired by Arnd and the commit
> 5f872175c867a839e2feeecfa59e3db38c88b561

where is that commit ?

$ git show 5f872175c867a839e2feeecfa59e3db38c88b561
fatal: bad object 5f872175c867a839e2feeecfa59e3db38c88b561

I have here linus and linux-next just fetched before trying to git show.

> For imitating that in my experiment recently, i only need to add clock
> on/off in dwc3 core.

Let's wait a little bit more before doing that. We have a ton of users
which don't have controllable clocks and, frankly speaking, if we hide
all clock control on glue layer (which is a parent device of core) it
will still work just fine because of the ordering of children vs parent
that device core gives you for free :-)

> Then dwc3 core won't be always created via sub node in platform glue node,
> and vendors like us can just drop platform glue which don't have any
> specific platform code to wrap dwc3 core, and just directly use dwc3 core
> via device tree.

If your core is really just xhci, why don't just use xhci-plat.c
directly ? You don't need dwc3 at all. In any case, I refrain from
allowing people to use dwc3.ko directly because that has bitten me in
the past with MUSB, so sorry but I'll always force people to use the
glue layer as that helps me keep dwc3 clean of platform-specific
details.

> Of course here dwc3 core won't process clock if he got null one, so this
> won't affect other vendor's platform which only pass clock phandle to their
> dwc3 platform glue driver.

right

> Just wondering if it's better to write a platform glue only did clock
> on/off, or to do the same improvement people ever did for
> ehci-platform driver and just drop this platform glue driver?

I'll always the sour taste in my mouth due to MUSB. The thing is a
complete mess of platform-specific hacks all over the place.

> Hope I didn't reply too late, or make any impolite move here,

merge window just closed, we've got time.

> Super thanks for every comment you made here.

hey, no problem.

--
balbi

Attachment: signature.asc
Description: Digital signature