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

[Felipe Balbi]

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.

> +};
> +
> +/* return 0 means successful */

this comment is unnecessary :-)

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

> +		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().

> +
> +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);


> +	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();

> +	}
> +
> +	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.

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 ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature