Re: [PATCH v2 1/5] drivers: usb: chipidea: Add qoriq platform driver

[Peter Chen <hzpeterchen@gmail.com>]

On Sat, Jul 09, 2016 at 10:00:52AM +0530, Rajesh Bhagat wrote:
> Adds qoriq platform driver for chipidea controller,
> verfied on LS1021A and LS1012A platforms.
> 
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> ---
> Changes in v2:
>  - Replaced Freescale with QorIQ in comments section
>  - Added macros to remove hardcoding while programming registers
>  - Changed the compatible string to fsl,ci-qoriq-usb2 and added version
>  - Removed calls to devm free/release calls 
> 
>  drivers/usb/chipidea/Makefile        |   2 +
>  drivers/usb/chipidea/ci_hdrc_qoriq.c | 237 +++++++++++++++++++++++++++++++++++
>  drivers/usb/chipidea/ci_hdrc_qoriq.h |  65 ++++++++++
>  3 files changed, 304 insertions(+)
>  create mode 100644 drivers/usb/chipidea/ci_hdrc_qoriq.c
>  create mode 100644 drivers/usb/chipidea/ci_hdrc_qoriq.h
> 
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 518e445..3122b86b 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -14,3 +14,5 @@ obj-$(CONFIG_USB_CHIPIDEA)	+= ci_hdrc_zevio.o
>  obj-$(CONFIG_USB_CHIPIDEA_PCI)	+= ci_hdrc_pci.o
>  
>  obj-$(CONFIG_USB_CHIPIDEA_OF)	+= usbmisc_imx.o ci_hdrc_imx.o
> +
> +obj-$(CONFIG_USB_CHIPIDEA)      += ci_hdrc_qoriq.o
> diff --git a/drivers/usb/chipidea/ci_hdrc_qoriq.c b/drivers/usb/chipidea/ci_hdrc_qoriq.c
> new file mode 100644
> index 0000000..3f478c6
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci_hdrc_qoriq.c
> @@ -0,0 +1,237 @@
> +/*
> + * QorIQ SoC USB 2.0 Controller driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + * Author: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/usb/of.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/clk.h>
> +
> +#include "ci.h"
> +#include "ci_hdrc_qoriq.h"
> +
> +struct ci_hdrc_qoriq_data {
> +	struct phy *phy;
> +	struct clk *clk;
> +	void __iomem *qoriq_regs;
> +	struct platform_device *ci_pdev;
> +	enum usb_phy_interface phy_mode;
> +};
> +
> +/*
> + * clock helper functions
> + */
> +static int ci_hdrc_qoriq_get_clks(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct ci_hdrc_qoriq_data *data = platform_get_drvdata(pdev);
> +
> +	data->clk = devm_clk_get(dev, "usb2-clock");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(dev, "failed to get clk, err=%ld\n",
> +					PTR_ERR(data->clk));
> +			return ret;

return PTR_ERR(data->clk), and delete ret.

> +	}
> +	return 0;
> +}
> +
> +static int ci_hdrc_qoriq_prepare_enable_clks(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct ci_hdrc_qoriq_data *data = platform_get_drvdata(pdev);
> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to prepare/enable clk, err=%d\n", ret);
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void ci_hdrc_qoriq_disable_unprepare_clks(struct platform_device *pdev)
> +{
> +	struct ci_hdrc_qoriq_data *data = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(data->clk);
> +}
> +
> +static int ci_hdrc_qoriq_usb_setup(struct platform_device *pdev)
> +{
> +	u32 reg;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct ci_hdrc_qoriq_data *data = platform_get_drvdata(pdev);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "failed to get I/O memory\n");
> +		return -ENOENT;
> +	}
> +
> +	dev_dbg(dev, "res->start %llx, resource_size(res) %llx\n", res->start,
> +		resource_size(res));

Delete above debug message please.

> +	data->qoriq_regs = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(data->qoriq_regs)) {
> +		dev_err(dev, "failed to remap I/O memory\n");
> +		return -ENOMEM;
> +	}
> +

This mapping will be freed soon, using ioremap instead.

> +	data->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
> +	dev_dbg(dev, "phy_mode %d\n", data->phy_mode);

Delete above debug message please.

> +
> +	reg = ioread32be(data->qoriq_regs + QORIQ_SOC_USB_CTRL);
> +	switch (data->phy_mode) {
> +	case USBPHY_INTERFACE_MODE_ULPI:
> +		iowrite32be(reg | ~UTMI_PHY_EN,
> +			    data->qoriq_regs + QORIQ_SOC_USB_CTRL);
> +		reg = ioread32be(data->qoriq_regs + QORIQ_SOC_USB_CTRL);
> +		iowrite32be(reg | USB_CTRL_USB_EN,
> +			    data->qoriq_regs + QORIQ_SOC_USB_CTRL);
> +		break;
> +	default:
> +		dev_err(dev, "unsupported phy_mode %d\n", data->phy_mode);
> +		return -EINVAL;
> +	}
> +
> +	/* Setup Snooping for all the 4GB space */
> +	/* SNOOP1 starts from 0x0, size 2G */
> +	iowrite32be(SNOOP_SIZE_2GB, data->qoriq_regs + QORIQ_SOC_USB_SNOOP1);
> +	/* SNOOP2 starts from 0x80000000, size 2G */
> +	iowrite32be(SNOOP_SIZE_2GB | 0x80000000,
> +		data->qoriq_regs + QORIQ_SOC_USB_SNOOP2);

What does this snoop mean?

> +
> +	iowrite32be(PRICTRL_PRI_LVL, data->qoriq_regs + QORIQ_SOC_USB_PRICTRL);
> +	iowrite32be(AGECNTTHRSH_THRESHOLD, data->qoriq_regs +
> +		    QORIQ_SOC_USB_AGECNTTHRSH);
> +	iowrite32be(SICTRL_RD_PREFETCH_32_BYTE, data->qoriq_regs +
> +		    QORIQ_SOC_USB_SICTRL);
> +
> +	devm_iounmap(dev, data->qoriq_regs);

iounmap.

> +	return 0;
> +}
> +
> +static int ci_hdrc_qoriq_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct ci_hdrc_qoriq_data *data;
> +	struct ci_hdrc_platform_data pdata = {
> +		.name		= dev_name(dev),
> +		.capoffset	= DEF_CAPOFFSET,
> +		.flags		= CI_HDRC_DISABLE_STREAMING,
> +	};
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	ret = ci_hdrc_qoriq_get_clks(pdev);
> +	if (ret)
> +		goto err_out;
> +
> +	ret = ci_hdrc_qoriq_prepare_enable_clks(pdev);
> +	if (ret)
> +		goto err_out;

Why not return directly?

> +
> +	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(dev, "failed to set coherent dma mask, err=%d\n", ret);
> +		goto err_clks;
> +	}

This dma setting does not be needed, the device tree will set it when
the device is created.

> +
> +	ret = ci_hdrc_qoriq_usb_setup(pdev);
> +	if (ret) {
> +		dev_err(dev, "failed to perform qoriq_usb2 setup, err=%d\n",
> +			ret);
> +		goto err_clks;
> +	}
> +
> +	data->phy = devm_phy_get(dev, "usb2-phy");
> +	if (IS_ERR(data->phy)) {
> +		ret = PTR_ERR(data->phy);
> +		/* Return -EINVAL if no usbphy is available */
> +		if (ret == -ENODEV)
> +			ret = -EINVAL;
> +		dev_err(dev, "failed get phy device, err=%d\n", ret);
> +		goto err_clks;
> +	}
> +	pdata.phy = data->phy;

The chipidea core will do that.

> +
> +	data->ci_pdev = ci_hdrc_add_device(dev,
> +				pdev->resource, pdev->num_resources,
> +				&pdata);
> +	if (IS_ERR(data->ci_pdev)) {
> +		ret = PTR_ERR(data->ci_pdev);
> +		dev_err(dev,
> +			"failed to register ci_hdrc platform device, err=%d\n",
> +			ret);
> +		goto err_clks;
> +	}
> +
> +	pm_runtime_no_callbacks(dev);
> +	pm_runtime_enable(dev);
> +
> +	dev_dbg(dev, "initialized\n");
> +	return 0;
> +
> +err_clks:
> +	ci_hdrc_qoriq_disable_unprepare_clks(pdev);

If you have only one clock, it is unnecessary to use dedicated APIs
for clock operation.

> +err_out:
> +	return ret;
> +}
> +
> +static int ci_hdrc_qoriq_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ci_hdrc_qoriq_data *data = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(dev);
> +	ci_hdrc_remove_device(data->ci_pdev);
> +	ci_hdrc_qoriq_disable_unprepare_clks(pdev);
> +	dev_dbg(dev, "de-initialized\n");
> +	return 0;
> +}
> +
> +static void ci_hdrc_qoriq_shutdown(struct platform_device *pdev)
> +{
> +	ci_hdrc_qoriq_remove(pdev);
> +}
> +
> +static const struct of_device_id ci_hdrc_qoriq_dt_ids[] = {
> +	{ .compatible = "fsl,ci-qoriq-usb2"},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ci_hdrc_qoriq_dt_ids);
> +
> +static struct platform_driver ci_hdrc_qoriq_driver = {
> +	.probe = ci_hdrc_qoriq_probe,
> +	.remove = ci_hdrc_qoriq_remove,
> +	.shutdown = ci_hdrc_qoriq_shutdown,
> +	.driver = {
> +		.name = "ci_qoriq_usb2",
> +		.of_match_table = ci_hdrc_qoriq_dt_ids,
> +	 },
> +};
> +
> +module_platform_driver(ci_hdrc_qoriq_driver);
> +
> +MODULE_ALIAS("platform:ci-qoriq-usb2");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("CI HDRC QORIQ USB binding");
> +MODULE_AUTHOR("Rajesh Bhagat <rajesh.bhagat@nxp.com>");
> diff --git a/drivers/usb/chipidea/ci_hdrc_qoriq.h b/drivers/usb/chipidea/ci_hdrc_qoriq.h
> new file mode 100644
> index 0000000..afd29442
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci_hdrc_qoriq.h
> @@ -0,0 +1,65 @@
> +/*
> + * QorIQ SoC USB 2.0 Controller driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + * Author: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#ifndef __DRIVER_USB_CHIPIDEA_CI_HDRC_QORIQ_H
> +#define __DRIVER_USB_CHIPIDEA_CI_HDRC_QORIQ_H
> +
> +/* offsets for the non-ehci registers in the QORIQ SOC USB controller */
> +#define QORIQ_SOC_USB_SBUSCFG	0x90
> +#define SBUSCFG_INCR8		0x02	/* INCR8, specified */
> +#define QORIQ_SOC_USB_ULPIVP	0x170
> +#define QORIQ_SOC_USB_PORTSC1	0x184
> +#define PORT_PTS_MSK		(3<<30)
> +#define PORT_PTS_UTMI		(0<<30)
> +#define PORT_PTS_ULPI		(2<<30)
> +#define	PORT_PTS_SERIAL		(3<<30)
> +#define PORT_PTS_PTW		(1<<28)
> +#define QORIQ_SOC_USB_PORTSC2	0x188
> +#define QORIQ_SOC_USB_USBMODE	0x1a8
> +#define USBMODE_CM_MASK		(3 << 0)	/* controller mode mask */
> +#define USBMODE_CM_HOST		(3 << 0)	/* controller mode: host */
> +#define USBMODE_ES		(1 << 2)	/* (Big) Endian Select */
> +
> +#define QORIQ_SOC_USB_USBGENCTRL	0x200
> +#define USBGENCTRL_PPP		(1 << 3)
> +#define USBGENCTRL_PFP		(1 << 2)
> +#define QORIQ_SOC_USB_ISIPHYCTRL	0x204
> +#define ISIPHYCTRL_PXE		(1)
> +#define ISIPHYCTRL_PHYE		(1 << 4)
> +
> +#define QORIQ_SOC_USB_SNOOP1		0x400	/* NOTE: big-endian */
> +#define QORIQ_SOC_USB_SNOOP2		0x404	/* NOTE: big-endian */
> +#define QORIQ_SOC_USB_AGECNTTHRSH	0x408	/* NOTE: big-endian */
> +#define AGECNTTHRSH_THRESHOLD		0x40
> +#define QORIQ_SOC_USB_PRICTRL		0x40c	/* NOTE: big-endian */
> +#define PRICTRL_PRI_LVL			0xc
> +#define QORIQ_SOC_USB_SICTRL		0x410	/* NOTE: big-endian */
> +#define SICTRL_RD_PREFETCH_32_BYTE	(0x1)
> +#define SICTRL_RD_PREFETCH_64_BYTE	(0x0)
> +#define QORIQ_SOC_USB_CTRL		0x500	/* NOTE: big-endian */
> +#define CTRL_UTMI_PHY_EN		(1 << 9)
> +#define CTRL_PHY_CLK_VALID		(1 << 17)
> +#define SNOOP_SIZE_2GB			0x1e
> +
> +/* control Register Bit Masks */
> +#define CONTROL_REGISTER_W1C_MASK       0x00020000  /* W1C: PHY_CLK_VALID */
> +#define ULPI_INT_EN             (1<<0)
> +#define WU_INT_EN               (1<<1)
> +#define USB_CTRL_USB_EN         (1<<2)
> +#define LINE_STATE_FILTER__EN   (1<<3)
> +#define KEEP_OTG_ON             (1<<4)
> +#define OTG_PORT                (1<<5)
> +#define PLL_RESET               (1<<8)
> +#define UTMI_PHY_EN             (1<<9)
> +#define ULPI_PHY_CLK_SEL        (1<<10)
> +#define PHY_CLK_VALID		(1<<17)
> +

Move necessary definitions to source file, and delete this header file
please, this header file is only used by ci_hdrc_qoriq.c. 

-- 

Best Regards,
Peter Chen