[RFC,v2,04/15] usb:cdns3: Driver initialization code.

Commit Message

Pawel Laszczak Nov. 18, 2018, 10:09 a.m. UTC

Patch adds core.c and core.h file that implements initialization
of platform driver and adds function responsible for selecting,
switching and running appropriate Device/Host mode.

Signed-off-by: Pawel Laszczak <[e-mail]>
---
 drivers/usb/cdns3/Makefile |   2 +
 drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
 drivers/usb/cdns3/core.h   | 100 +++++++++
 3 files changed, 515 insertions(+)
 create mode 100644 drivers/usb/cdns3/core.c
 create mode 100644 drivers/usb/cdns3/core.h

Comments

Roger Quadros Nov. 23, 2018, 11:35 a.m. UTC | #1

On 18/11/18 12:09, Pawel Laszczak wrote:
> Patch adds core.c and core.h file that implements initialization
> of platform driver and adds function responsible for selecting,
> switching and running appropriate Device/Host mode.
> 
> Signed-off-by: Pawel Laszczak <[e-mail]>
> ---
>  drivers/usb/cdns3/Makefile |   2 +
>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>  drivers/usb/cdns3/core.h   | 100 +++++++++
>  3 files changed, 515 insertions(+)
>  create mode 100644 drivers/usb/cdns3/core.c
>  create mode 100644 drivers/usb/cdns3/core.h
> 
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index dcdd62003c6a..02d25b23c5d3 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -1,3 +1,5 @@
> +obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci.o
>  
> +cdns3-y					:= core.o
>  cdns3-pci-y		 		:= cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> new file mode 100644
> index 000000000000..f9055d4da67f
> --- /dev/null
> +++ b/drivers/usb/cdns3/core.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Peter Chen <[e-mail]>
> + *         Pawel Laszczak <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "gadget.h"
> +#include "core.h"
> +
> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> +{
> +	WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> +	return cdns->roles[cdns->role];
> +}
> +
> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
> +{
> +	int ret;
> +
> +	if (role >= CDNS3_ROLE_END)

WARN_ON()?

> +		return 0;
> +
> +	if (!cdns->roles[role])
> +		return -ENXIO;
> +
> +	mutex_lock(&cdns->mutex);
> +	cdns->role = role;
> +	ret = cdns->roles[role]->start(cdns);
> +	mutex_unlock(&cdns->mutex);
> +	return ret;
> +}
> +
> +static inline void cdns3_role_stop(struct cdns3 *cdns)
> +{
> +	enum cdns3_roles role = cdns->role;
> +
> +	if (role == CDNS3_ROLE_END)

WARN_ON(role >= CNDS3_ROLE_END) ?

> +		return;
> +
> +	mutex_lock(&cdns->mutex);
> +	cdns->roles[role]->stop(cdns);
> +	cdns->role = CDNS3_ROLE_END;

Why change the role here? You are just stopping the role not changing it.
I think cdns->role should remain unchanged, so we can call cdns3_role_start()
if required without error.

> +	mutex_unlock(&cdns->mutex);
> +}
> +
> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> +{
> +	if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> +		//TODO: implements selecting device/host mode
> +		return CDNS3_ROLE_HOST;
> +	}
> +	return cdns->roles[CDNS3_ROLE_HOST]
> +		? CDNS3_ROLE_HOST
> +		: CDNS3_ROLE_GADGET;

Why not just
	return cdns->role;

I'm wondering if we really need this function.
> +}

> +
> +/**
> + * cdns3_core_init_role - initialize role of operation
> + * @cdns: Pointer to cdns3 structure
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_core_init_role(struct cdns3 *cdns)
> +{
> +	struct device *dev = cdns->dev;
> +	enum usb_dr_mode dr_mode;
> +
> +	dr_mode = usb_get_dr_mode(dev);
> +	cdns->role = CDNS3_ROLE_END;
> +
> +	/*
> +	 * If driver can't read mode by means of usb_get_dr_mdoe function then
> +	 * chooses mode according with Kernel configuration. This setting
> +	 * can be restricted later depending on strap pin configuration.
> +	 */
> +	if (dr_mode == USB_DR_MODE_UNKNOWN) {
> +		if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> +		    IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> +			dr_mode = USB_DR_MODE_OTG;
> +		else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> +			dr_mode = USB_DR_MODE_HOST;
> +		else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> +			dr_mode = USB_DR_MODE_PERIPHERAL;
> +	}
> +
> +	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> +		//TODO: implements host initialization

		/* TODO: Add host role */ ?

> +	}
> +
> +	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
> +		//TODO: implements device initialization

		/* TODO: Add device role */ ?

> +	}
> +
> +	if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
> +		dev_err(dev, "no supported roles\n");
> +		return -ENODEV;
> +	}
> +
> +	cdns->dr_mode = dr_mode;
> +	return 0;
> +}
> +
> +/**
> + * cdns3_irq - interrupt handler for cdns3 core device
> + *
> + * @irq: irq number for cdns3 core device
> + * @data: structure of cdns3
> + *
> + * Returns IRQ_HANDLED or IRQ_NONE
> + */
> +static irqreturn_t cdns3_irq(int irq, void *data)
> +{
> +	struct cdns3 *cdns = data;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	/* Handle device/host interrupt */
> +	if (cdns->role != CDNS3_ROLE_END)

Is it because of this that you need to set role to END at role_stop?
I think it is better to add a state variable to struct cdns3_role_driver, so we can
check if it is active or stopped.

e.g.
	if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)

> +		ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
> +
> +	return ret;
> +}
> +
> +static void cdns3_remove_roles(struct cdns3 *cdns)

Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?

> +{
> +	//TODO: implements this function
> +}

> +
> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
> +{
> +	enum cdns3_roles current_role;
> +	int ret = 0;
> +
> +	current_role = cdns->role;
> +
> +	if (role == CDNS3_ROLE_END)
> +		return 0;

role == END looks like error state. and it should never happen.
WARN here?

> +
> +	dev_dbg(cdns->dev, "Switching role");
> +

Don't you have to stop the previous role before starting the new role?

> +	ret = cdns3_role_start(cdns, role);
> +	if (ret) {
> +		/* Back to current role */
> +		dev_err(cdns->dev, "set %d has failed, back to %d\n",
> +			role, current_role);
> +		ret = cdns3_role_start(cdns, current_role);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * cdns3_role_switch - work queue handler for role switch
> + *
> + * @work: work queue item structure
> + *
> + * Handles below events:
> + * - Role switch for dual-role devices
> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
> + */
> +static void cdns3_role_switch(struct work_struct *work)
> +{
> +	enum cdns3_roles role = CDNS3_ROLE_END;
> +	struct cdns3 *cdns;
> +	bool device, host;
> +
> +	cdns = container_of(work, struct cdns3, role_switch_wq);
> +
> +	//TODO: implements this functions.
> +	//host = cdns3_is_host(cdns);
> +	//device = cdns3_is_device(cdns);
> +	host = 1;
> +	device = 0;
> +
> +	if (host)
> +		role = CDNS3_ROLE_HOST;
> +	else if (device)
> +		role = CDNS3_ROLE_GADGET;
> +
> +	if (cdns->desired_dr_mode == cdns->current_dr_mode &&
> +	    cdns->role == role)
> +		return;
> +

I think all the below code can be moved to cdns3_do_role_switch().

> +	pm_runtime_get_sync(cdns->dev);
> +	cdns3_role_stop(cdns);
> +
> +	if (host) {
> +		if (cdns->roles[CDNS3_ROLE_HOST])
> +			cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
> +		pm_runtime_put_sync(cdns->dev);
> +		return;
> +	}
> +
> +	if (device)
> +		cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
> +	else
> +		cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
> +
> +	pm_runtime_put_sync(cdns->dev);
> +}
> +
> +/**
> + * cdns3_probe - probe for cdns3 core device
> + * @pdev: Pointer to cdns3 core platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource	*res;
> +	struct cdns3 *cdns;
> +	void __iomem *regs;
> +	int ret;
> +
> +	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> +	if (!cdns)
> +		return -ENOMEM;
> +
> +	cdns->dev = dev;
> +
> +	platform_set_drvdata(pdev, cdns);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res) {
> +		dev_err(dev, "missing IRQ\n");
> +		return -ENODEV;
> +	}
> +	cdns->irq = res->start;
> +
> +	/*
> +	 * Request memory region
> +	 * region-0: xHCI
> +	 * region-1: Peripheral
> +	 * region-2: OTG registers
> +	 */

The memory region order is different from the dt-binding.
There it is OTG, host(xhci), device (peripheral).

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(dev, res);
> +
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +	cdns->xhci_regs = regs;
> +	cdns->xhci_res = res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +	cdns->dev_regs	= regs;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +	cdns->otg_regs = regs;
> +
> +	mutex_init(&cdns->mutex);
> +
> +	cdns->phy = devm_phy_get(dev, "cdns3,usbphy");

"cdns3,usbphy" is not documented in dt-binding.

> +	if (IS_ERR(cdns->phy)) {
> +		dev_info(dev, "no generic phy found\n");
> +		cdns->phy = NULL;
> +		/*
> +		 * fall through here!
> +		 * if no generic phy found, phy init
> +		 * should be done under boot!
> +		 */

No you shouldn't fall through always if it is an error condition.
Something like this should work better.

        if (IS_ERR(cnds->phy)) {
                ret = PTR_ERR(cdns->phy);
                if (ret == -ENOSYS || ret == -ENODEV) {
                        cdns->phy = NULL;
                } else if (ret == -EPROBE_DEFER) {
                        return ret;
                } else {
                        dev_err(dev, "no phy found\n");
                        goto err0;
                }
        }

So if PHY was provided in DT, and PHY support/drivers is present
and error condition means something is wrong and we have to error out.

> +	} else {
> +		phy_init(cdns->phy);
> +	}

You can do phy_init() outside the else.

> +
> +	ret = cdns3_core_init_role(cdns);
> +	if (ret)
> +		goto err1;
> +
> +	INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
> +	if (ret)
> +		goto err2;
> +
> +	if (ret)
> +		goto err2;
> +
> +	cdns->role = cdns3_get_role(cdns);

I think this should move to cdns3_core_init_role().

> +
> +	ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
> +			       dev_name(dev), cdns);
> +
> +	if (ret)
> +		goto err2;

How about moving request_irq to before cdsn3_core_init_role()?

Then you can move cdns3_role_start() as well to core_init_role().

> +
> +	ret = cdns3_role_start(cdns, cdns->role);
> +	if (ret) {
> +		dev_err(dev, "can't start %s role\n",
> +			cdns3_get_current_role_driver(cdns)->name);
> +		goto err2;
> +	}
> +
> +	device_set_wakeup_capable(dev, true);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	/*
> +	 * The controller needs less time between bus and controller suspend,
> +	 * and we also needs a small delay to avoid frequently entering low
> +	 * power mode.
> +	 */
> +	pm_runtime_set_autosuspend_delay(dev, 20);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_use_autosuspend(dev);
> +	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
> +
> +	return 0;
> +
> +err2:
> +	cdns3_remove_roles(cdns);
> +err1:

phy_exit() ?

> +	return ret;
> +}
> +
> +/**
> + * cdns3_remove - unbind drd driver and clean up
> + * @pdev: Pointer to Linux platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_remove(struct platform_device *pdev)
> +{
> +	struct cdns3 *cdns = platform_get_drvdata(pdev);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +	cdns3_remove_roles(cdns);

phy_exit() ?

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cdns3_match[] = {
> +	{ .compatible = "cdns,usb3" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
> +#endif
> +
> +#ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cdns3_suspend(struct device *dev)
> +{
> +	//TODO: implements this function
> +	return 0;
> +}
> +
> +static int cdns3_resume(struct device *dev)
> +{
> +	//TODO: implements this function
> +	return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +static int cdns3_runtime_suspend(struct device *dev)
> +{	//TODO: implements this function
> +	return 0;
> +}
> +
> +static int cdns3_runtime_resume(struct device *dev)
> +{
> +	//TODO: implements this function
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops cdns3_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
> +	SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver cdns3_driver = {
> +	.probe		= cdns3_probe,
> +	.remove		= cdns3_remove,
> +	.driver		= {
> +		.name	= "cdns-usb3",
> +		.of_match_table	= of_match_ptr(of_cdns3_match),
> +		.pm	= &cdns3_pm_ops,
> +	},
> +};
> +
> +static int __init cdns3_driver_platform_register(void)
> +{
> +	return platform_driver_register(&cdns3_driver);
> +}
> +module_init(cdns3_driver_platform_register);
> +
> +static void __exit cdns3_driver_platform_unregister(void)
> +{
> +	platform_driver_unregister(&cdns3_driver);
> +}
> +module_exit(cdns3_driver_platform_unregister);
> +
> +MODULE_ALIAS("platform:cdns3");
> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> new file mode 100644
> index 000000000000..7c8204fe4d3d
> --- /dev/null
> +++ b/drivers/usb/cdns3/core.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2017 NXP
> + * Copyright (C) 2018 Cadence.
> + *
> + * Authors: Peter Chen <[e-mail]>
> + *          Pawel Laszczak <[e-mail]>
> + */
> +#include <linux/usb/otg.h>
> +
> +#ifndef __LINUX_CDNS3_CORE_H
> +#define __LINUX_CDNS3_CORE_H
> +
> +struct cdns3;
> +enum cdns3_roles {
> +	CDNS3_ROLE_HOST = 0,
> +	CDNS3_ROLE_GADGET,
> +	CDNS3_ROLE_END,
> +};
> +
> +/**
> + * struct cdns3_role_driver - host/gadget role driver
> + * @start: start this role
> + * @stop: stop this role
> + * @suspend: suspend callback for this role
> + * @resume: resume callback for this role
> + * @irq: irq handler for this role
> + * @name: role name string (host/gadget)
> + */
> +struct cdns3_role_driver {
> +	int (*start)(struct cdns3 *cdns);
> +	void (*stop)(struct cdns3 *cdns);
> +	int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
> +	int (*resume)(struct cdns3 *cdns, bool hibernated);
> +	irqreturn_t (*irq)(struct cdns3 *cdns);
> +	const char *name;
> +};
> +
> +#define CDNS3_NUM_OF_CLKS	5
> +/**
> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
> + * @dev: pointer to Cadence device struct
> + * @xhci_regs: pointer to base of xhci registers
> + * @xhci_res: the resource for xhci
> + * @dev_regs: pointer to base of dev registers
> + * @otg_regs: pointer to base of otg registers
> + * @irq: irq number for controller
> + * @roles: array of supported roles for this controller
> + * @role: current role
> + * @host_dev: the child host device pointer for cdns3 core
> + * @gadget_dev: the child gadget device pointer for cdns3 core
> + * @usb: phy for this controller
> + * @role_switch_wq: work queue item for role switch
> + * @in_lpm: the controller in low power mode
> + * @wakeup_int: the wakeup interrupt
> + * @mutex: the mutex for concurrent code at driver
> + * @dr_mode: supported mode of operation it can be only Host, only Device
> + *           or OTG mode that allow to switch between Device and Host mode.
> + *           This field based on hardware configuration and cant't be changed.

But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.

> + * @current_dr_role: current mode of operation when in dual-role mode
> + * @desired_dr_role: desired mode of operation when in dual-role mode.
> + *           This value can be changed during runtime.
> + *           Available options depends on  dr_mode:
> + *           dr_mode                 |  desired_dr_role and current_dr_role
> + *           ----------------------------------------------------------------
> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG

Do you need to update the right hand side to reflect ROLEs instead of MODE?

> + *
> + *           Desired_dr_role can be changed by means of debugfs.
> + * @root: debugfs root folder pointer
> + */
> +struct cdns3 {
> +	struct device			*dev;
> +	void __iomem			*xhci_regs;
> +	struct resource			*xhci_res;
> +	struct cdns3_usb_regs __iomem	*dev_regs;
> +	struct cdns3_otg_regs		*otg_regs;
> +	int irq;
> +	struct cdns3_role_driver	*roles[CDNS3_ROLE_END];
> +	enum cdns3_roles		role;
> +	struct device			*host_dev;
> +	struct device			*gadget_dev;
> +	struct phy			*phy;
> +	struct work_struct		role_switch_wq;
> +	int				in_lpm:1;
> +	int				wakeup_int:1;
> +	/* mutext used in workqueue*/
> +	struct mutex			mutex;
> +	enum usb_dr_mode		dr_mode;
> +	enum usb_dr_mode		current_dr_mode;
> +	enum usb_dr_mode		desired_dr_mode;
> +	struct dentry			*root;
> +};
> +
> +#endif /* __LINUX_CDNS3_CORE_H */
> 

cheers,
-roger

Pawel Laszczak Nov. 25, 2018, 12:35 p.m. UTC | #2

Hi Roger

>On 18/11/18 12:09, Pawel Laszczak wrote:
>> Patch adds core.c and core.h file that implements initialization
>> of platform driver and adds function responsible for selecting,
>> switching and running appropriate Device/Host mode.
>>
>> Signed-off-by: Pawel Laszczak <[e-mail]>
>> ---
>>  drivers/usb/cdns3/Makefile |   2 +
>>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>>  drivers/usb/cdns3/core.h   | 100 +++++++++
>>  3 files changed, 515 insertions(+)
>>  create mode 100644 drivers/usb/cdns3/core.c
>>  create mode 100644 drivers/usb/cdns3/core.h
>>
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index dcdd62003c6a..02d25b23c5d3 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -1,3 +1,5 @@
>> +obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
>>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci.o
>>
>> +cdns3-y					:= core.o
>>  cdns3-pci-y		 		:= cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> new file mode 100644
>> index 000000000000..f9055d4da67f
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -0,0 +1,413 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Peter Chen <[e-mail]>
>> + *         Pawel Laszczak <[e-mail]>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include "gadget.h"
>> +#include "core.h"
>> +
>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>> +{
>> +	WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>> +	return cdns->roles[cdns->role];
>> +}
>> +
>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>> +{
>> +	int ret;
>> +
>> +	if (role >= CDNS3_ROLE_END)
>
>WARN_ON()?
I agree. 
>
>> +		return 0;
>> +
>> +	if (!cdns->roles[role])
>> +		return -ENXIO;
>> +
>> +	mutex_lock(&cdns->mutex);
>> +	cdns->role = role;
>> +	ret = cdns->roles[role]->start(cdns);
>> +	mutex_unlock(&cdns->mutex);
>> +	return ret;
>> +}
>> +
>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> +{
>> +	enum cdns3_roles role = cdns->role;
>> +
>> +	if (role == CDNS3_ROLE_END)
>
>WARN_ON(role >= CNDS3_ROLE_END) ?
I agree
>
>> +		return;
>> +
>> +	mutex_lock(&cdns->mutex);
>> +	cdns->roles[role]->stop(cdns);
>> +	cdns->role = CDNS3_ROLE_END;
>
>Why change the role here? You are just stopping the role not changing it.
>I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>if required without error.

This line is unnecessary.

>
>> +	mutex_unlock(&cdns->mutex);
>> +}
>> +
>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> +{
>> +	if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> +		//TODO: implements selecting device/host mode
>> +		return CDNS3_ROLE_HOST;
>> +	}
>> +	return cdns->roles[CDNS3_ROLE_HOST]
>> +		? CDNS3_ROLE_HOST
>> +		: CDNS3_ROLE_GADGET;
>
>Why not just
>	return cdns->role;
>
>I'm wondering if we really need this function

TODO will look likie:
		if (cdns3_is_host(cdns))
			return CDNS3_ROLE_HOST;
		if (cdns3_is_device(cdns))
			return CDNS3_ROLE_GADGET;

Function selects initial role. Before invoking it the role is unknown.
I think that function name should be  changed because current name can be misleading. 

I will change it to cdns3_get_initial_role.
.
>> +}
>
>> +
>> +/**
>> + * cdns3_core_init_role - initialize role of operation
>> + * @cdns: Pointer to cdns3 structure
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_core_init_role(struct cdns3 *cdns)
>> +{
>> +	struct device *dev = cdns->dev;
>> +	enum usb_dr_mode dr_mode;
>> +
>> +	dr_mode = usb_get_dr_mode(dev);
>> +	cdns->role = CDNS3_ROLE_END;
>> +
>> +	/*
>> +	 * If driver can't read mode by means of usb_get_dr_mdoe function then
>> +	 * chooses mode according with Kernel configuration. This setting
>> +	 * can be restricted later depending on strap pin configuration.
>> +	 */
>> +	if (dr_mode == USB_DR_MODE_UNKNOWN) {
>> +		if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>> +		    IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> +			dr_mode = USB_DR_MODE_OTG;
>> +		else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>> +			dr_mode = USB_DR_MODE_HOST;
>> +		else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> +			dr_mode = USB_DR_MODE_PERIPHERAL;
>> +	}
>> +
>> +	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> +		//TODO: implements host initialization
>
>		/* TODO: Add host role */ ?
>
>> +	}
>> +
>> +	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>> +		//TODO: implements device initialization
>
>		/* TODO: Add device role */ ?
>
>> +	}
>> +
>> +	if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>> +		dev_err(dev, "no supported roles\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	cdns->dr_mode = dr_mode;
>> +	return 0;
>> +}
>> +
>> +/**
>> + * cdns3_irq - interrupt handler for cdns3 core device
>> + *
>> + * @irq: irq number for cdns3 core device
>> + * @data: structure of cdns3
>> + *
>> + * Returns IRQ_HANDLED or IRQ_NONE
>> + */
>> +static irqreturn_t cdns3_irq(int irq, void *data)
>> +{
>> +	struct cdns3 *cdns = data;
>> +	irqreturn_t ret = IRQ_NONE;
>> +
>> +	/* Handle device/host interrupt */
>> +	if (cdns->role != CDNS3_ROLE_END)
>
>Is it because of this that you need to set role to END at role_stop?
>I think it is better to add a state variable to struct cdns3_role_driver, so we can
>check if it is active or stopped.
>
>e.g.
>	if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)

Ok, I will do it in this way. 
>> +		ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>> +
>> +	return ret;
>> +}
>> +
>> +static void cdns3_remove_roles(struct cdns3 *cdns)
>
>Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?

Sounds better. 
I also change cdns3_host_remove to cdns3_host_exit and
cdns3_gadget_remove to cdns3_gadget_exit. 
>
>> +{
>> +	//TODO: implements this function
>> +}
>
>> +
>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>> +{
>> +	enum cdns3_roles current_role;
>> +	int ret = 0;
>> +
>> +	current_role = cdns->role;
>> +
>> +	if (role == CDNS3_ROLE_END)
>> +		return 0;
>
>role == END looks like error state. and it should never happen.
>WARN here?

Ok, will be changed.
>
>> +
>> +	dev_dbg(cdns->dev, "Switching role");
>> +
>
>Don't you have to stop the previous role before starting the new role?
>
>> +	ret = cdns3_role_start(cdns, role);
>> +	if (ret) {
>> +		/* Back to current role */
>> +		dev_err(cdns->dev, "set %d has failed, back to %d\n",
>> +			role, current_role);
>> +		ret = cdns3_role_start(cdns, current_role);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * cdns3_role_switch - work queue handler for role switch
>> + *
>> + * @work: work queue item structure
>> + *
>> + * Handles below events:
>> + * - Role switch for dual-role devices
>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>> + */
>> +static void cdns3_role_switch(struct work_struct *work)
>> +{
>> +	enum cdns3_roles role = CDNS3_ROLE_END;
>> +	struct cdns3 *cdns;
>> +	bool device, host;
>> +
>> +	cdns = container_of(work, struct cdns3, role_switch_wq);
>> +
>> +	//TODO: implements this functions.
>> +	//host = cdns3_is_host(cdns);
>> +	//device = cdns3_is_device(cdns);
>> +	host = 1;
>> +	device = 0;
>> +
>> +	if (host)
>> +		role = CDNS3_ROLE_HOST;
>> +	else if (device)
>> +		role = CDNS3_ROLE_GADGET;
>> +
>> +	if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>> +	    cdns->role == role)
>> +		return;
>> +
>
>I think all the below code can be moved to cdns3_do_role_switch().

Yes, I agree with you. cdns3_role_stop  should be in cdns3_do_role_switch. 

>> +	pm_runtime_get_sync(cdns->dev);
>> +	cdns3_role_stop(cdns);
>> +
>> +	if (host) {
>> +		if (cdns->roles[CDNS3_ROLE_HOST])
>> +			cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>> +		pm_runtime_put_sync(cdns->dev);
>> +		return;
>> +	}
>> +
>> +	if (device)
>> +		cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>> +	else
>> +		cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>> +
>> +	pm_runtime_put_sync(cdns->dev);
>> +}
>> +
>> +/**
>> + * cdns3_probe - probe for cdns3 core device
>> + * @pdev: Pointer to cdns3 core platform device
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource	*res;
>> +	struct cdns3 *cdns;
>> +	void __iomem *regs;
>> +	int ret;
>> +
>> +	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>> +	if (!cdns)
>> +		return -ENOMEM;
>> +
>> +	cdns->dev = dev;
>> +
>> +	platform_set_drvdata(pdev, cdns);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +	if (!res) {
>> +		dev_err(dev, "missing IRQ\n");
>> +		return -ENODEV;
>> +	}
>> +	cdns->irq = res->start;
>> +
>> +	/*
>> +	 * Request memory region
>> +	 * region-0: xHCI
>> +	 * region-1: Peripheral
>> +	 * region-2: OTG registers
>> +	 */
>
>The memory region order is different from the dt-binding.
>There it is OTG, host(xhci), device (peripheral).

I corrected dt-binding.
>
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	regs = devm_ioremap_resource(dev, res);
>> +
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +	cdns->xhci_regs = regs;
>> +	cdns->xhci_res = res;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +	cdns->dev_regs	= regs;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +	regs = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +	cdns->otg_regs = regs;
>> +
>> +	mutex_init(&cdns->mutex);
>> +
>> +	cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>
>"cdns3,usbphy" is not documented in dt-binding.

I assume that I should add  to dt-binding (cdns3-usb.txt) something like:
 - phys: reference to the USB PHY
 - phy-names: name of the USB PHY, should be " cdns3,usbphy "
 
>
>> +	if (IS_ERR(cdns->phy)) {
>> +		dev_info(dev, "no generic phy found\n");
>> +		cdns->phy = NULL;
>> +		/*
>> +		 * fall through here!
>> +		 * if no generic phy found, phy init
>> +		 * should be done under boot!
>> +		 */
>
>No you shouldn't fall through always if it is an error condition.
>Something like this should work better.
>
>        if (IS_ERR(cnds->phy)) {
>                ret = PTR_ERR(cdns->phy);
>                if (ret == -ENOSYS || ret == -ENODEV) {
>                        cdns->phy = NULL;
>                } else if (ret == -EPROBE_DEFER) {
>                        return ret;
>                } else {
>                        dev_err(dev, "no phy found\n");
>                        goto err0;
>                }
>        }
>
>So if PHY was provided in DT, and PHY support/drivers is present
>and error condition means something is wrong and we have to error out.
>
>> +	} else {
>> +		phy_init(cdns->phy);
>> +	}
>
>You can do phy_init() outside the else.
>
Thank you for explanation. I will correct this. 
>> +
>> +	ret = cdns3_core_init_role(cdns);
>> +	if (ret)
>> +		goto err1;
>> +
>> +	INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>> +	if (ret)
>> +		goto err2;
>> +
>> +	if (ret)
>> +		goto err2;
>> +
>> +	cdns->role = cdns3_get_role(cdns);
>
>I think this should move to cd I'll have a some though on ns3_core_init_role().

Ok, I will do it. 
>
>> +
>> +	ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>> +			       dev_name(dev), cdns);
>> +
>> +	if (ret)
>> +		goto err2;
>
>How about moving request_irq to before cdsn3_core_init_role()?
>
>Then you can move cdns3_role_start() as well to core_init_role().
I'll give it  some though on it, but probably I will probably have to change little other function..
So the new order should look like this: 

cdns3_drd_init
devm_request_irq
cdns3_core_init_role, cdns3_get_role, cdns3_role_start

>
>> +
>> +	ret = cdns3_role_start(cdns, cdns->role);
>> +	if (ret) {
>> +		dev_err(dev, "can't start %s role\n",
>> +			cdns3_get_current_role_driver(cdns)->name);
>> +		goto err2;
>> +	}
>> +
>> +	device_set_wakeup_capable(dev, true);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	/*
>> +	 * The controller needs less time between bus and controller suspend,
>> +	 * and we also needs a small delay to avoid frequently entering low
>> +	 * power mode.
>> +	 */
>> +	pm_runtime_set_autosuspend_delay(dev, 20);
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_use_autosuspend(dev);
>> +	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
>> +
>> +	return 0;
>> +
>> +err2:
>> +	cdns3_remove_roles(cdns);
>> +err1:
>
>phy_exit() ?
I will add. 
>
>> +	return ret;
>> +}
>> +
>> +/**
>> + * cdns3_remove - unbind drd driver and clean up
>> + * @pdev: Pointer to Linux platform device
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_remove(struct platform_device *pdev)
>> +{
>> +	struct cdns3 *cdns = platform_get_drvdata(pdev);
>> +
>> +	pm_runtime_get_sync(&pdev->dev);
>> +	pm_runtime_disable(&pdev->dev);
>> +	pm_runtime_put_noidle(&pdev->dev);
>> +	cdns3_remove_roles(cdns);
>
>phy_exit() ?
I will add.
>
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_cdns3_match[] = {
>> +	{ .compatible = "cdns,usb3" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
>> +#endif
>> +
>> +#ifdef CONFIG_PM
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cdns3_suspend(struct device *dev)
>> +{
>> +	//TODO: implements this function
>> +	return 0;
>> +}
>> +
>> +static int cdns3_resume(struct device *dev)
>> +{
>> +	//TODO: implements this function
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
>> +static int cdns3_runtime_suspend(struct device *dev)
>> +{	//TODO: implements this function
>> +	return 0;
>> +}
>> +
>> +static int cdns3_runtime_resume(struct device *dev)
>> +{
>> +	//TODO: implements this function
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops cdns3_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
>> +	SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
>> +};
>> +
>> +static struct platform_driver cdns3_driver = {
>> +	.probe		= cdns3_probe,
>> +	.remove		= cdns3_remove,
>> +	.driver		= {
>> +		.name	= "cdns-usb3",
>> +		.of_match_table	= of_match_ptr(of_cdns3_match),
>> +		.pm	= &cdns3_pm_ops,
>> +	},
>> +};
>> +
>> +static int __init cdns3_driver_platform_register(void)
>> +{
>> +	return platform_driver_register(&cdns3_driver);
>> +}
>> +module_init(cdns3_driver_platform_register);
>> +
>> +static void __exit cdns3_driver_platform_unregister(void)
>> +{
>> +	platform_driver_unregister(&cdns3_driver);
>> +}
>> +module_exit(cdns3_driver_platform_unregister);
>> +
>> +MODULE_ALIAS("platform:cdns3");
>> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> new file mode 100644
>> index 000000000000..7c8204fe4d3d
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/core.h
>> @@ -0,0 +1,100 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2017 NXP
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Authors: Peter Chen <[e-mail]>
>> + *          Pawel Laszczak <[e-mail]>
>> + */
>> +#include <linux/usb/otg.h>
>> +
>> +#ifndef __LINUX_CDNS3_CORE_H
>> +#define __LINUX_CDNS3_CORE_H
>> +
>> +struct cdns3;
>> +enum cdns3_roles {
>> +	CDNS3_ROLE_HOST = 0,
>> +	CDNS3_ROLE_GADGET,
>> +	CDNS3_ROLE_END,
>> +};
>> +
>> +/**
>> + * struct cdns3_role_driver - host/gadget role driver
>> + * @start: start this role
>> + * @stop: stop this role
>> + * @suspend: suspend callback for this role
>> + * @resume: resume callback for this role
>> + * @irq: irq handler for this role
>> + * @name: role name string (host/gadget)
>> + */
>> +struct cdns3_role_driver {
>> +	int (*start)(struct cdns3 *cdns);
>> +	void (*stop)(struct cdns3 *cdns);
>> +	int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
>> +	int (*resume)(struct cdns3 *cdns, bool hibernated);
>> +	irqreturn_t (*irq)(struct cdns3 *cdns);
>> +	const char *name;
>> +};
>> +
>> +#define CDNS3_NUM_OF_CLKS	5
>> +/**
>> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
>> + * @dev: pointer to Cadence device struct
>> + * @xhci_regs: pointer to base of xhci registers
>> + * @xhci_res: the resource for xhci
>> + * @dev_regs: pointer to base of dev registers
>> + * @otg_regs: pointer to base of otg registers
>> + * @irq: irq number for controller
>> + * @roles: array of supported roles for this controller
>> + * @role: current role
>> + * @host_dev: the child host device pointer for cdns3 core
>> + * @gadget_dev: the child gadget device pointer for cdns3 core
>> + * @usb: phy for this controller
>> + * @role_switch_wq: work queue item for role switch
>> + * @in_lpm: the controller in low power mode
>> + * @wakeup_int: the wakeup interrupt
>> + * @mutex: the mutex for concurrent code at driver
>> + * @dr_mode: supported mode of operation it can be only Host, only Device
>> + *           or OTG mode that allow to switch between Device and Host mode.
>> + *           This field based on hardware configuration and cant't be changed.
>
>But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.
Right, I added dr_mode to dt-binding , so we have STRAP bits in registers and additionally 
optional dr_mode in device-tree. Driver should take into account this two options.   
I will remove this line. 

>
>> + * @current_dr_role: current mode of operation when in dual-role mode
>> + * @desired_dr_role: desired mode of operation when in dual-role mode.
>> + *           This value can be changed during runtime.
>> + *           Available options depends on  dr_mode:
>> + *           dr_mode                 |  desired_dr_role and current_dr_role
>> + *           ----------------------------------------------------------------
>> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
>> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
>> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
>> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
>> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
>
>Do you need to update the right hand side to reflect ROLEs instead of MODE?

I see that there are incorrect name. There should be mode instead role. 
In structure below the names are correct. 
>
>> + *
>> + *           Desired_dr_role can be changed by means of debugfs.
>> + * @root: debugfs root folder pointer
>> + */
>> +struct cdns3 {
>> +	struct device			*dev;
>> +	void __iomem			*xhci_regs;
>> +	struct resource			*xhci_res;
>> +	struct cdns3_usb_regs __iomem	*dev_regs;
>> +	struct cdns3_otg_regs		*otg_regs;
>> +	int irq;
>> +	struct cdns3_role_driver	*roles[CDNS3_ROLE_END];
>> +	enum cdns3_roles		role;
>> +	struct device			*host_dev;
>> +	struct device			*gadget_dev;
>> +	struct phy			*phy;
>> +	struct work_struct		role_switch_wq;
>> +	int				in_lpm:1;
>> +	int				wakeup_int:1;
>> +	/* mutext used in workqueue*/
>> +	struct mutex			mutex;
>> +	enum usb_dr_mode		dr_mode;
>> +	enum usb_dr_mode		current_dr_mode;
>> +	enum usb_dr_mode		desired_dr_mode;
>> +	struct dentry			*root;
>> +};
>> +
>> +#endif /* __LINUX_CDNS3_CORE_H */
>>
>
>cheers,
>-roger
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Thank for all your comments,
Cheers,
Pawel

Peter Chen Nov. 30, 2018, 7:32 a.m. UTC | #3

On Sun, Nov 18, 2018 at 6:16 PM Pawel Laszczak <[e-mail]> wrote:
>
> Patch adds core.c and core.h file that implements initialization
> of platform driver and adds function responsible for selecting,
> switching and running appropriate Device/Host mode.
>
> Signed-off-by: Pawel Laszczak <[e-mail]>
> ---
>  drivers/usb/cdns3/Makefile |   2 +
>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>  drivers/usb/cdns3/core.h   | 100 +++++++++
>  3 files changed, 515 insertions(+)
>  create mode 100644 drivers/usb/cdns3/core.c
>  create mode 100644 drivers/usb/cdns3/core.h
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index dcdd62003c6a..02d25b23c5d3 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -1,3 +1,5 @@
> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
>
> +cdns3-y                                        := core.o
>  cdns3-pci-y                            := cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> new file mode 100644
> index 000000000000..f9055d4da67f
> --- /dev/null
> +++ b/drivers/usb/cdns3/core.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018 Cadence.
> + *

Please add NXP copyright too.

> + * Author: Peter Chen <[e-mail]>
> + *         Pawel Laszczak <[e-mail]>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "gadget.h"
> +#include "core.h"
> +
> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> +{
> +       WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> +       return cdns->roles[cdns->role];
> +}
> +

Can we delete "current", and use cdns3_get_role_driver directly?

> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
> +{
> +       int ret;
> +
> +       if (role >= CDNS3_ROLE_END)
> +               return 0;
> +
> +       if (!cdns->roles[role])
> +               return -ENXIO;
> +
> +       mutex_lock(&cdns->mutex);
> +       cdns->role = role;
> +       ret = cdns->roles[role]->start(cdns);
> +       mutex_unlock(&cdns->mutex);
> +       return ret;
> +}
> +
> +static inline void cdns3_role_stop(struct cdns3 *cdns)
> +{
> +       enum cdns3_roles role = cdns->role;
> +
> +       if (role == CDNS3_ROLE_END)
> +               return;
> +
> +       mutex_lock(&cdns->mutex);
> +       cdns->roles[role]->stop(cdns);
> +       cdns->role = CDNS3_ROLE_END;
> +       mutex_unlock(&cdns->mutex);
> +}
> +
> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> +{
> +       if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> +               //TODO: implements selecting device/host mode
> +               return CDNS3_ROLE_HOST;
> +       }
> +       return cdns->roles[CDNS3_ROLE_HOST]
> +               ? CDNS3_ROLE_HOST
> +               : CDNS3_ROLE_GADGET;
> +}
> +
> +/**
> + * cdns3_core_init_role - initialize role of operation
> + * @cdns: Pointer to cdns3 structure
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_core_init_role(struct cdns3 *cdns)
> +{
> +       struct device *dev = cdns->dev;
> +       enum usb_dr_mode dr_mode;
> +
> +       dr_mode = usb_get_dr_mode(dev);
> +       cdns->role = CDNS3_ROLE_END;
> +
> +       /*
> +        * If driver can't read mode by means of usb_get_dr_mdoe function then
> +        * chooses mode according with Kernel configuration. This setting
> +        * can be restricted later depending on strap pin configuration.
> +        */
> +       if (dr_mode == USB_DR_MODE_UNKNOWN) {
> +               if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> +                   IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> +                       dr_mode = USB_DR_MODE_OTG;
> +               else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> +                       dr_mode = USB_DR_MODE_HOST;
> +               else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> +                       dr_mode = USB_DR_MODE_PERIPHERAL;
> +       }
> +
> +       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> +               //TODO: implements host initialization
> +       }
> +
> +       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
> +               //TODO: implements device initialization
> +       }
> +
> +       if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
> +               dev_err(dev, "no supported roles\n");
> +               return -ENODEV;
> +       }
> +
> +       cdns->dr_mode = dr_mode;
> +       return 0;
> +}
> +
> +/**
> + * cdns3_irq - interrupt handler for cdns3 core device
> + *
> + * @irq: irq number for cdns3 core device
> + * @data: structure of cdns3
> + *
> + * Returns IRQ_HANDLED or IRQ_NONE
> + */
> +static irqreturn_t cdns3_irq(int irq, void *data)
> +{
> +       struct cdns3 *cdns = data;
> +       irqreturn_t ret = IRQ_NONE;
> +
> +       /* Handle device/host interrupt */
> +       if (cdns->role != CDNS3_ROLE_END)
> +               ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
> +
> +       return ret;
> +}
> +
> +static void cdns3_remove_roles(struct cdns3 *cdns)
> +{
> +       //TODO: implements this function
> +}
> +
> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
> +{
> +       enum cdns3_roles current_role;
> +       int ret = 0;
> +
> +       current_role = cdns->role;
> +
> +       if (role == CDNS3_ROLE_END)
> +               return 0;
> +
> +       dev_dbg(cdns->dev, "Switching role");
> +
> +       ret = cdns3_role_start(cdns, role);
> +       if (ret) {
> +               /* Back to current role */
> +               dev_err(cdns->dev, "set %d has failed, back to %d\n",
> +                       role, current_role);
> +               ret = cdns3_role_start(cdns, current_role);
> +       }
> +
> +       return ret;
> +}
> +
> +/**
> + * cdns3_role_switch - work queue handler for role switch
> + *
> + * @work: work queue item structure
> + *
> + * Handles below events:
> + * - Role switch for dual-role devices
> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
> + */
> +static void cdns3_role_switch(struct work_struct *work)
> +{
> +       enum cdns3_roles role = CDNS3_ROLE_END;
> +       struct cdns3 *cdns;
> +       bool device, host;
> +
> +       cdns = container_of(work, struct cdns3, role_switch_wq);
> +
> +       //TODO: implements this functions.
> +       //host = cdns3_is_host(cdns);
> +       //device = cdns3_is_device(cdns);

You may improve use C comment.

> +       host = 1;
> +       device = 0;
> +
> +       if (host)
> +               role = CDNS3_ROLE_HOST;
> +       else if (device)
> +               role = CDNS3_ROLE_GADGET;
> +
> +       if (cdns->desired_dr_mode == cdns->current_dr_mode &&
> +           cdns->role == role)
> +               return;
> +
> +       pm_runtime_get_sync(cdns->dev);
> +       cdns3_role_stop(cdns);
> +
> +       if (host) {
> +               if (cdns->roles[CDNS3_ROLE_HOST])
> +                       cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
> +               pm_runtime_put_sync(cdns->dev);
> +               return;
> +       }
> +
> +       if (device)
> +               cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
> +       else
> +               cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
> +
> +       pm_runtime_put_sync(cdns->dev);
> +}
> +
> +/**
> + * cdns3_probe - probe for cdns3 core device
> + * @pdev: Pointer to cdns3 core platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       struct cdns3 *cdns;
> +       void __iomem *regs;
> +       int ret;
> +
> +       cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> +       if (!cdns)
> +               return -ENOMEM;
> +
> +       cdns->dev = dev;
> +
> +       platform_set_drvdata(pdev, cdns);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (!res) {
> +               dev_err(dev, "missing IRQ\n");
> +               return -ENODEV;
> +       }
> +       cdns->irq = res->start;
> +
> +       /*
> +        * Request memory region
> +        * region-0: xHCI
> +        * region-1: Peripheral
> +        * region-2: OTG registers
> +        */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       regs = devm_ioremap_resource(dev, res);
> +
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +       cdns->xhci_regs = regs;
> +       cdns->xhci_res = res;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       regs = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +       cdns->dev_regs  = regs;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +       regs = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +       cdns->otg_regs = regs;
> +
> +       mutex_init(&cdns->mutex);
> +
> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> +       if (IS_ERR(cdns->phy)) {
> +               dev_info(dev, "no generic phy found\n");
> +               cdns->phy = NULL;
> +               /*
> +                * fall through here!
> +                * if no generic phy found, phy init
> +                * should be done under boot!
> +                */

If the phy driver is defer-probed, it will be here, it is not an error.
I think you could have a generic phy driver or usb generic phy driver
(drivers/usb/phy/phy-generic.c) even you don't need any operations for
PHY. It will be easy for other platforms.

> +       } else {
> +               phy_init(cdns->phy);
> +       }
> +
> +       ret = cdns3_core_init_role(cdns);
> +       if (ret)
> +               goto err1;
> +
> +       INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
> +       if (ret)
> +               goto err2;
> +
> +       if (ret)
> +               goto err2;
> +
> +       cdns->role = cdns3_get_role(cdns);
> +
> +       ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
> +                              dev_name(dev), cdns);
> +
> +       if (ret)
> +               goto err2;
> +
> +       ret = cdns3_role_start(cdns, cdns->role);
> +       if (ret) {
> +               dev_err(dev, "can't start %s role\n",
> +                       cdns3_get_current_role_driver(cdns)->name);
> +               goto err2;
> +       }
> +
> +       device_set_wakeup_capable(dev, true);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
> +
> +       /*
> +        * The controller needs less time between bus and controller suspend,
> +        * and we also needs a small delay to avoid frequently entering low
> +        * power mode.
> +        */
> +       pm_runtime_set_autosuspend_delay(dev, 20);
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_use_autosuspend(dev);
> +       dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
> +
> +       return 0;
> +
> +err2:
> +       cdns3_remove_roles(cdns);
> +err1:
> +       return ret;
> +}
> +
> +/**
> + * cdns3_remove - unbind drd driver and clean up
> + * @pdev: Pointer to Linux platform device
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_remove(struct platform_device *pdev)
> +{
> +       struct cdns3 *cdns = platform_get_drvdata(pdev);
> +
> +       pm_runtime_get_sync(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
> +       cdns3_remove_roles(cdns);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_cdns3_match[] = {
> +       { .compatible = "cdns,usb3" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
> +#endif
> +
> +#ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cdns3_suspend(struct device *dev)
> +{
> +       //TODO: implements this function
> +       return 0;
> +}
> +
> +static int cdns3_resume(struct device *dev)
> +{
> +       //TODO: implements this function
> +       return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +static int cdns3_runtime_suspend(struct device *dev)
> +{      //TODO: implements this function
> +       return 0;
> +}
> +
> +static int cdns3_runtime_resume(struct device *dev)
> +{
> +       //TODO: implements this function
> +       return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops cdns3_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
> +       SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver cdns3_driver = {
> +       .probe          = cdns3_probe,
> +       .remove         = cdns3_remove,
> +       .driver         = {
> +               .name   = "cdns-usb3",
> +               .of_match_table = of_match_ptr(of_cdns3_match),
> +               .pm     = &cdns3_pm_ops,
> +       },
> +};
> +
> +static int __init cdns3_driver_platform_register(void)
> +{
> +       return platform_driver_register(&cdns3_driver);
> +}
> +module_init(cdns3_driver_platform_register);
> +
> +static void __exit cdns3_driver_platform_unregister(void)
> +{
> +       platform_driver_unregister(&cdns3_driver);
> +}
> +module_exit(cdns3_driver_platform_unregister);
> +
> +MODULE_ALIAS("platform:cdns3");
> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> new file mode 100644
> index 000000000000..7c8204fe4d3d
> --- /dev/null
> +++ b/drivers/usb/cdns3/core.h
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence USBSS DRD Driver.
> + *

Header file

> + * Copyright (C) 2017 NXP
> + * Copyright (C) 2018 Cadence.
> + *
> + * Authors: Peter Chen <[e-mail]>
> + *          Pawel Laszczak <[e-mail]>
> + */
> +#include <linux/usb/otg.h>
> +
> +#ifndef __LINUX_CDNS3_CORE_H
> +#define __LINUX_CDNS3_CORE_H
> +
> +struct cdns3;
> +enum cdns3_roles {
> +       CDNS3_ROLE_HOST = 0,
> +       CDNS3_ROLE_GADGET,
> +       CDNS3_ROLE_END,
> +};
> +
> +/**
> + * struct cdns3_role_driver - host/gadget role driver
> + * @start: start this role
> + * @stop: stop this role
> + * @suspend: suspend callback for this role
> + * @resume: resume callback for this role
> + * @irq: irq handler for this role
> + * @name: role name string (host/gadget)
> + */
> +struct cdns3_role_driver {
> +       int (*start)(struct cdns3 *cdns);
> +       void (*stop)(struct cdns3 *cdns);
> +       int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
> +       int (*resume)(struct cdns3 *cdns, bool hibernated);
> +       irqreturn_t (*irq)(struct cdns3 *cdns);
> +       const char *name;
> +};
> +
> +#define CDNS3_NUM_OF_CLKS      5
> +/**
> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
> + * @dev: pointer to Cadence device struct
> + * @xhci_regs: pointer to base of xhci registers
> + * @xhci_res: the resource for xhci
> + * @dev_regs: pointer to base of dev registers
> + * @otg_regs: pointer to base of otg registers
> + * @irq: irq number for controller
> + * @roles: array of supported roles for this controller
> + * @role: current role
> + * @host_dev: the child host device pointer for cdns3 core
> + * @gadget_dev: the child gadget device pointer for cdns3 core
> + * @usb: phy for this controller
> + * @role_switch_wq: work queue item for role switch
> + * @in_lpm: the controller in low power mode
> + * @wakeup_int: the wakeup interrupt
> + * @mutex: the mutex for concurrent code at driver
> + * @dr_mode: supported mode of operation it can be only Host, only Device
> + *           or OTG mode that allow to switch between Device and Host mode.
> + *           This field based on hardware configuration and cant't be changed.

Based on firmware setting, kernel configuration and hardware configuration.

> + * @current_dr_role: current mode of operation when in dual-role mode
> + * @desired_dr_role: desired mode of operation when in dual-role mode.
> + *           This value can be changed during runtime.
> + *           Available options depends on  dr_mode:
> + *           dr_mode                 |  desired_dr_role and current_dr_role
> + *           ----------------------------------------------------------------
> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
> + *
> + *           Desired_dr_role can be changed by means of debugfs.
> + * @root: debugfs root folder pointer
> + */
> +struct cdns3 {
> +       struct device                   *dev;
> +       void __iomem                    *xhci_regs;
> +       struct resource                 *xhci_res;
> +       struct cdns3_usb_regs __iomem   *dev_regs;
> +       struct cdns3_otg_regs           *otg_regs;
> +       int irq;
> +       struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
> +       enum cdns3_roles                role;
> +       struct device                   *host_dev;
> +       struct device                   *gadget_dev;
> +       struct phy                      *phy;
> +       struct work_struct              role_switch_wq;
> +       int                             in_lpm:1;
> +       int                             wakeup_int:1;
> +       /* mutext used in workqueue*/
> +       struct mutex                    mutex;
> +       enum usb_dr_mode                dr_mode;
> +       enum usb_dr_mode                current_dr_mode;
> +       enum usb_dr_mode                desired_dr_mode;
> +       struct dentry                   *root;
> +};
> +
> +#endif /* __LINUX_CDNS3_CORE_H */
> --
> 2.17.1
>

Pawel Laszczak Dec. 2, 2018, 8:34 p.m. UTC | #4

>>
>> Patch adds core.c and core.h file that implements initialization
>> of platform driver and adds function responsible for selecting,
>> switching and running appropriate Device/Host mode.
>>
>> Signed-off-by: Pawel Laszczak <[e-mail]>
>> ---
>>  drivers/usb/cdns3/Makefile |   2 +
>>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>>  drivers/usb/cdns3/core.h   | 100 +++++++++
>>  3 files changed, 515 insertions(+)
>>  create mode 100644 drivers/usb/cdns3/core.c
>>  create mode 100644 drivers/usb/cdns3/core.h
>>
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> index dcdd62003c6a..02d25b23c5d3 100644
>> --- a/drivers/usb/cdns3/Makefile
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -1,3 +1,5 @@
>> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
>>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
>>
>> +cdns3-y                                        := core.o
>>  cdns3-pci-y                            := cdns3-pci-wrap.o
>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> new file mode 100644
>> index 000000000000..f9055d4da67f
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/core.c
>> @@ -0,0 +1,413 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>
>Please add NXP copyright too.

Ok, I don't know why I omitted this. 
I know that you are the main author of this file 
Sorry for that.

One additional question. What year I should add in Copyright for NXP?. 
The original year 2017 or I should modified all to 2018. 

>> + * Author: Peter Chen <[e-mail]>
>> + *         Pawel Laszczak <[e-mail]>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#include "gadget.h"
>> +#include "core.h"
>> +
>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>> +{
>> +       WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>> +       return cdns->roles[cdns->role];
>> +}
>> +
>
>Can we delete "current", and use cdns3_get_role_driver directly?

Yes, sure. Role is always current.
>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>> +{
>> +       int ret;
>> +
>> +       if (role >= CDNS3_ROLE_END)
>> +               return 0;
>> +
>> +       if (!cdns->roles[role])
>> +               return -ENXIO;
>> +
>> +       mutex_lock(&cdns->mutex);
>> +       cdns->role = role;
>> +       ret = cdns->roles[role]->start(cdns);
>> +       mutex_unlock(&cdns->mutex);
>> +       return ret;
>> +}
>> +
>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> +{
>> +       enum cdns3_roles role = cdns->role;
>> +
>> +       if (role == CDNS3_ROLE_END)
>> +               return;
>> +
>> +       mutex_lock(&cdns->mutex);
>> +       cdns->roles[role]->stop(cdns);
>> +       cdns->role = CDNS3_ROLE_END;
>> +       mutex_unlock(&cdns->mutex);
>> +}
>> +
>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> +{
>> +       if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> +               //TODO: implements selecting device/host mode
>> +               return CDNS3_ROLE_HOST;
>> +       }
>> +       return cdns->roles[CDNS3_ROLE_HOST]
>> +               ? CDNS3_ROLE_HOST
>> +               : CDNS3_ROLE_GADGET;
>> +}
>> +
>> +/**
>> + * cdns3_core_init_role - initialize role of operation
>> + * @cdns: Pointer to cdns3 structure
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_core_init_role(struct cdns3 *cdns)
>> +{
>> +       struct device *dev = cdns->dev;
>> +       enum usb_dr_mode dr_mode;
>> +
>> +       dr_mode = usb_get_dr_mode(dev);
>> +       cdns->role = CDNS3_ROLE_END;
>> +
>> +       /*
>> +        * If driver can't read mode by means of usb_get_dr_mdoe function then
>> +        * chooses mode according with Kernel configuration. This setting
>> +        * can be restricted later depending on strap pin configuration.
>> +        */
>> +       if (dr_mode == USB_DR_MODE_UNKNOWN) {
>> +               if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>> +                   IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> +                       dr_mode = USB_DR_MODE_OTG;
>> +               else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>> +                       dr_mode = USB_DR_MODE_HOST;
>> +               else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> +                       dr_mode = USB_DR_MODE_PERIPHERAL;
>> +       }
>> +
>> +       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> +               //TODO: implements host initialization
>> +       }
>> +
>> +       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>> +               //TODO: implements device initialization
>> +       }
>> +
>> +       if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>> +               dev_err(dev, "no supported roles\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       cdns->dr_mode = dr_mode;
>> +       return 0;
>> +}
>> +
>> +/**
>> + * cdns3_irq - interrupt handler for cdns3 core device
>> + *
>> + * @irq: irq number for cdns3 core device
>> + * @data: structure of cdns3
>> + *
>> + * Returns IRQ_HANDLED or IRQ_NONE
>> + */
>> +static irqreturn_t cdns3_irq(int irq, void *data)
>> +{
>> +       struct cdns3 *cdns = data;
>> +       irqreturn_t ret = IRQ_NONE;
>> +
>> +       /* Handle device/host interrupt */
>> +       if (cdns->role != CDNS3_ROLE_END)
>> +               ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>> +
>> +       return ret;
>> +}
>> +
>> +static void cdns3_remove_roles(struct cdns3 *cdns)
>> +{
>> +       //TODO: implements this function
>> +}
>> +
>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>> +{
>> +       enum cdns3_roles current_role;
>> +       int ret = 0;
>> +
>> +       current_role = cdns->role;
>> +
>> +       if (role == CDNS3_ROLE_END)
>> +               return 0;
>> +
>> +       dev_dbg(cdns->dev, "Switching role");
>> +
>> +       ret = cdns3_role_start(cdns, role);
>> +       if (ret) {
>> +               /* Back to current role */
>> +               dev_err(cdns->dev, "set %d has failed, back to %d\n",
>> +                       role, current_role);
>> +               ret = cdns3_role_start(cdns, current_role);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>> + * cdns3_role_switch - work queue handler for role switch
>> + *
>> + * @work: work queue item structure
>> + *
>> + * Handles below events:
>> + * - Role switch for dual-role devices
>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>> + */
>> +static void cdns3_role_switch(struct work_struct *work)
>> +{
>> +       enum cdns3_roles role = CDNS3_ROLE_END;
>> +       struct cdns3 *cdns;
>> +       bool device, host;
>> +
>> +       cdns = container_of(work, struct cdns3, role_switch_wq);
>> +
>> +       //TODO: implements this functions.
>> +       //host = cdns3_is_host(cdns);
>> +       //device = cdns3_is_device(cdns);
>
>You may improve use C comment.
Yes, I know. I've already done that. 
>
>> +       host = 1;
>> +       device = 0;
>> +
>> +       if (host)
>> +               role = CDNS3_ROLE_HOST;
>> +       else if (device)
>> +               role = CDNS3_ROLE_GADGET;
>> +
>> +       if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>> +           cdns->role == role)
>> +               return;
>> +
>> +       pm_runtime_get_sync(cdns->dev);
>> +       cdns3_role_stop(cdns);
>> +
>> +       if (host) {
>> +               if (cdns->roles[CDNS3_ROLE_HOST])
>> +                       cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>> +               pm_runtime_put_sync(cdns->dev);
>> +               return;
>> +       }
>> +
>> +       if (device)
>> +               cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>> +       else
>> +               cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>> +
>> +       pm_runtime_put_sync(cdns->dev);
>> +}
>> +
>> +/**
>> + * cdns3_probe - probe for cdns3 core device
>> + * @pdev: Pointer to cdns3 core platform device
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct resource *res;
>> +       struct cdns3 *cdns;
>> +       void __iomem *regs;
>> +       int ret;
>> +
>> +       cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>> +       if (!cdns)
>> +               return -ENOMEM;
>> +
>> +       cdns->dev = dev;
>> +
>> +       platform_set_drvdata(pdev, cdns);
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +       if (!res) {
>> +               dev_err(dev, "missing IRQ\n");
>> +               return -ENODEV;
>> +       }
>> +       cdns->irq = res->start;
>> +
>> +       /*
>> +        * Request memory region
>> +        * region-0: xHCI
>> +        * region-1: Peripheral
>> +        * region-2: OTG registers
>> +        */
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       regs = devm_ioremap_resource(dev, res);
>> +
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +       cdns->xhci_regs = regs;
>> +       cdns->xhci_res = res;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +       regs = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +       cdns->dev_regs  = regs;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +       regs = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +       cdns->otg_regs = regs;
>> +
>> +       mutex_init(&cdns->mutex);
>> +
>> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>> +       if (IS_ERR(cdns->phy)) {
>> +               dev_info(dev, "no generic phy found\n");
>> +               cdns->phy = NULL;
>> +               /*
>> +                * fall through here!
>> +                * if no generic phy found, phy init
>> +                * should be done under boot!
>> +                */
>
>If the phy driver is defer-probed, it will be here, it is not an error.
>I think you could have a generic phy driver or usb generic phy driver
>(drivers/usb/phy/phy-generic.c) even you don't need any operations for
>PHY. It will be easy for other platforms.

Yes, Roger ask me to modify this fragment. In next version it will look like:
	cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
	if (IS_ERR(cdns->phy)) {
		ret = PTR_ERR(cdns->phy);
		if (ret == -ENOSYS || ret == -ENODEV) {
			cdns->phy = NULL;
		} else if (ret == -EPROBE_DEFER) {
			return ret;
		} else {
			dev_err(dev, "no phy found\n");
			goto err0;
		}
	}

	phy_init(cdns->phy);

We are going to use phy driver. I don't know if it correct. 
I don't have experience in this filed.  
We need phy initialization but I don't have testing platform now.  
In most usb drivers I see that there are used usb phy driverd instead phy dirverd.

>> +       } else {
>> +               phy_init(cdns->phy);
>> +       }
>> +
>> +       ret = cdns3_core_init_role(cdns);
>> +       if (ret)
>> +               goto err1;
>> +
>> +       INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>> +       if (ret)
>> +               goto err2;
>> +
>> +       if (ret)
>> +               goto err2;
>> +
>> +       cdns->role = cdns3_get_role(cdns);
>> +
>> +       ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>> +                              dev_name(dev), cdns);
>> +
>> +       if (ret)
>> +               goto err2;
>> +
>> +       ret = cdns3_role_start(cdns, cdns->role);
>> +       if (ret) {
>> +               dev_err(dev, "can't start %s role\n",
>> +                       cdns3_get_current_role_driver(cdns)->name);
>> +               goto err2;
>> +       }
>> +
>> +       device_set_wakeup_capable(dev, true);
>> +       pm_runtime_set_active(dev);
>> +       pm_runtime_enable(dev);
>> +
>> +       /*
>> +        * The controller needs less time between bus and controller suspend,
>> +        * and we also needs a small delay to avoid frequently entering low
>> +        * power mode.
>> +        */
>> +       pm_runtime_set_autosuspend_delay(dev, 20);
>> +       pm_runtime_mark_last_busy(dev);
>> +       pm_runtime_use_autosuspend(dev);
>> +       dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
>> +
>> +       return 0;
>> +
>> +err2:
>> +       cdns3_remove_roles(cdns);
>> +err1:
>> +       return ret;
>> +}
>> +
>> +/**
>> + * cdns3_remove - unbind drd driver and clean up
>> + * @pdev: Pointer to Linux platform device
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_remove(struct platform_device *pdev)
>> +{
>> +       struct cdns3 *cdns = platform_get_drvdata(pdev);
>> +
>> +       pm_runtime_get_sync(&pdev->dev);
>> +       pm_runtime_disable(&pdev->dev);
>> +       pm_runtime_put_noidle(&pdev->dev);
>> +       cdns3_remove_roles(cdns);
>> +
>> +       return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_cdns3_match[] = {
>> +       { .compatible = "cdns,usb3" },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
>> +#endif
>> +
>> +#ifdef CONFIG_PM
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cdns3_suspend(struct device *dev)
>> +{
>> +       //TODO: implements this function
>> +       return 0;
>> +}
>> +
>> +static int cdns3_resume(struct device *dev)
>> +{
>> +       //TODO: implements this function
>> +       return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
>> +static int cdns3_runtime_suspend(struct device *dev)
>> +{      //TODO: implements this function
>> +       return 0;
>> +}
>> +
>> +static int cdns3_runtime_resume(struct device *dev)
>> +{
>> +       //TODO: implements this function
>> +       return 0;
>> +}
>> +#endif /* CONFIG_PM */
>> +
>> +static const struct dev_pm_ops cdns3_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
>> +       SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
>> +};
>> +
>> +static struct platform_driver cdns3_driver = {
>> +       .probe          = cdns3_probe,
>> +       .remove         = cdns3_remove,
>> +       .driver         = {
>> +               .name   = "cdns-usb3",
>> +               .of_match_table = of_match_ptr(of_cdns3_match),
>> +               .pm     = &cdns3_pm_ops,
>> +       },
>> +};
>> +
>> +static int __init cdns3_driver_platform_register(void)
>> +{
>> +       return platform_driver_register(&cdns3_driver);
>> +}
>> +module_init(cdns3_driver_platform_register);
>> +
>> +static void __exit cdns3_driver_platform_unregister(void)
>> +{
>> +       platform_driver_unregister(&cdns3_driver);
>> +}
>> +module_exit(cdns3_driver_platform_unregister);
>> +
>> +MODULE_ALIAS("platform:cdns3");
>> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> new file mode 100644
>> index 000000000000..7c8204fe4d3d
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/core.h
>> @@ -0,0 +1,100 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>
>Header file
I don't understand. What is wrong ?
>
>> + * Copyright (C) 2017 NXP
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Authors: Peter Chen <[e-mail]>
>> + *          Pawel Laszczak <[e-mail]>
>> + */
>> +#include <linux/usb/otg.h>
>> +
>> +#ifndef __LINUX_CDNS3_CORE_H
>> +#define __LINUX_CDNS3_CORE_H
>> +
>> +struct cdns3;
>> +enum cdns3_roles {
>> +       CDNS3_ROLE_HOST = 0,
>> +       CDNS3_ROLE_GADGET,
>> +       CDNS3_ROLE_END,
>> +};
>> +
>> +/**
>> + * struct cdns3_role_driver - host/gadget role driver
>> + * @start: start this role
>> + * @stop: stop this role
>> + * @suspend: suspend callback for this role
>> + * @resume: resume callback for this role
>> + * @irq: irq handler for this role
>> + * @name: role name string (host/gadget)
>> + */
>> +struct cdns3_role_driver {
>> +       int (*start)(struct cdns3 *cdns);
>> +       void (*stop)(struct cdns3 *cdns);
>> +       int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
>> +       int (*resume)(struct cdns3 *cdns, bool hibernated);
>> +       irqreturn_t (*irq)(struct cdns3 *cdns);
>> +       const char *name;
>> +};
>> +
>> +#define CDNS3_NUM_OF_CLKS      5
>> +/**
>> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
>> + * @dev: pointer to Cadence device struct
>> + * @xhci_regs: pointer to base of xhci registers
>> + * @xhci_res: the resource for xhci
>> + * @dev_regs: pointer to base of dev registers
>> + * @otg_regs: pointer to base of otg registers
>> + * @irq: irq number for controller
>> + * @roles: array of supported roles for this controller
>> + * @role: current role
>> + * @host_dev: the child host device pointer for cdns3 core
>> + * @gadget_dev: the child gadget device pointer for cdns3 core
>> + * @usb: phy for this controller
>> + * @role_switch_wq: work queue item for role switch
>> + * @in_lpm: the controller in low power mode
>> + * @wakeup_int: the wakeup interrupt
>> + * @mutex: the mutex for concurrent code at driver
>> + * @dr_mode: supported mode of operation it can be only Host, only Device
>> + *           or OTG mode that allow to switch between Device and Host mode.
>> + *           This field based on hardware configuration and cant't be changed.
>
>Based on firmware setting, kernel configuration and hardware configuration.
I removed this line but this will be better. 
Thanks.
>
>> + * @current_dr_role: current mode of operation when in dual-role mode
>> + * @desired_dr_role: desired mode of operation when in dual-role mode.
>> + *           This value can be changed during runtime.
>> + *           Available options depends on  dr_mode:
>> + *           dr_mode                 |  desired_dr_role and current_dr_role
>> + *           ----------------------------------------------------------------
>> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
>> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
>> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
>> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
>> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
>> + *
>> + *           Desired_dr_role can be changed by means of debugfs.
>> + * @root: debugfs root folder pointer
>> + */
>> +struct cdns3 {
>> +       struct device                   *dev;
>> +       void __iomem                    *xhci_regs;
>> +       struct resource                 *xhci_res;
>> +       struct cdns3_usb_regs __iomem   *dev_regs;
>> +       struct cdns3_otg_regs           *otg_regs;
>> +       int irq;
>> +       struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
>> +       enum cdns3_roles                role;
>> +       struct device                   *host_dev;
>> +       struct device                   *gadget_dev;
>> +       struct phy                      *phy;
>> +       struct work_struct              role_switch_wq;
>> +       int                             in_lpm:1;
>> +       int                             wakeup_int:1;
>> +       /* mutext used in workqueue*/
>> +       struct mutex                    mutex;
>> +       enum usb_dr_mode                dr_mode;
>> +       enum usb_dr_mode                current_dr_mode;
>> +       enum usb_dr_mode                desired_dr_mode;
>> +       struct dentry                   *root;
>> +};
>> +
>> +#endif /* __LINUX_CDNS3_CORE_H */
>> --
>> 2.17.1
>>
Thank 
Cheers
Pawel

Peter Chen Dec. 4, 2018, 7:11 a.m. UTC | #5

On Mon, Dec 3, 2018 at 4:34 AM Pawel Laszczak <[e-mail]> wrote:
>
> >>
> >> Patch adds core.c and core.h file that implements initialization
> >> of platform driver and adds function responsible for selecting,
> >> switching and running appropriate Device/Host mode.
> >>
> >> Signed-off-by: Pawel Laszczak <[e-mail]>
> >> ---
> >>  drivers/usb/cdns3/Makefile |   2 +
> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
> >>  3 files changed, 515 insertions(+)
> >>  create mode 100644 drivers/usb/cdns3/core.c
> >>  create mode 100644 drivers/usb/cdns3/core.h
> >>
> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> >> index dcdd62003c6a..02d25b23c5d3 100644
> >> --- a/drivers/usb/cdns3/Makefile
> >> +++ b/drivers/usb/cdns3/Makefile
> >> @@ -1,3 +1,5 @@
> >> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
> >>
> >> +cdns3-y                                        := core.o
> >>  cdns3-pci-y                            := cdns3-pci-wrap.o
> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> new file mode 100644
> >> index 000000000000..f9055d4da67f
> >> --- /dev/null
> >> +++ b/drivers/usb/cdns3/core.c
> >> @@ -0,0 +1,413 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Cadence USBSS DRD Driver.
> >> + *
> >> + * Copyright (C) 2018 Cadence.
> >> + *
> >
> >Please add NXP copyright too.
>
> Ok, I don't know why I omitted this.
> I know that you are the main author of this file
> Sorry for that.
>
> One additional question. What year I should add in Copyright for NXP?.
> The original year 2017 or I should modified all to 2018.
>
Please use below copyright, thanks.

Copyright 2017-2018 NXP



> >> +       mutex_init(&cdns->mutex);
> >> +
> >> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> >> +       if (IS_ERR(cdns->phy)) {
> >> +               dev_info(dev, "no generic phy found\n");
> >> +               cdns->phy = NULL;
> >> +               /*
> >> +                * fall through here!
> >> +                * if no generic phy found, phy init
> >> +                * should be done under boot!
> >> +                */
> >
> >If the phy driver is defer-probed, it will be here, it is not an error.
> >I think you could have a generic phy driver or usb generic phy driver
> >(drivers/usb/phy/phy-generic.c) even you don't need any operations for
> >PHY. It will be easy for other platforms.
>
> Yes, Roger ask me to modify this fragment. In next version it will look like:
>         cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>         if (IS_ERR(cdns->phy)) {
>                 ret = PTR_ERR(cdns->phy);
>                 if (ret == -ENOSYS || ret == -ENODEV) {
>                         cdns->phy = NULL;
>                 } else if (ret == -EPROBE_DEFER) {
>                         return ret;
>                 } else {
>                         dev_err(dev, "no phy found\n");
>                         goto err0;
>                 }
>         }
>
>         phy_init(cdns->phy);
>
> We are going to use phy driver. I don't know if it correct.
> I don't have experience in this filed.
> We need phy initialization but I don't have testing platform now.
> In most usb drivers I see that there are used usb phy driverd instead phy dirverd.
>

At my CDNS3 platform, there are some USB PHY initialization for register setting
and clock enable. You could add generic PHY driver under: drivers/phy/cadence/.

Above PHY initialization code is OK for me.


> >> +static void __exit cdns3_driver_platform_unregister(void)
> >> +{
> >> +       platform_driver_unregister(&cdns3_driver);
> >> +}
> >> +module_exit(cdns3_driver_platform_unregister);
> >> +
> >> +MODULE_ALIAS("platform:cdns3");
> >> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> >> new file mode 100644
> >> index 000000000000..7c8204fe4d3d
> >> --- /dev/null
> >> +++ b/drivers/usb/cdns3/core.h
> >> @@ -0,0 +1,100 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Cadence USBSS DRD Driver.
> >> + *
> >
> >Header file
> I don't understand. What is wrong ?
> >

The comment for this file

Cadence USBSS DRD Core Header File

Peter

Peter Chen Dec. 4, 2018, 8:50 a.m. UTC | #6

> > + * Cadence USBSS DRD Driver.
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Peter Chen <[e-mail]>
> > + *         Pawel Laszczak <[e-mail]>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#include "gadget.h"
> > +#include "core.h"
> > +
> > +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> > +{
> > +     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> > +     return cdns->roles[cdns->role];
> > +}
> > +
> > +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
> > +{
> > +     int ret;
> > +
> > +     if (role >= CDNS3_ROLE_END)
>
> WARN_ON()?
>
> > +             return 0;
> > +
> > +     if (!cdns->roles[role])
> > +             return -ENXIO;
> > +
> > +     mutex_lock(&cdns->mutex);
> > +     cdns->role = role;
> > +     ret = cdns->roles[role]->start(cdns);
> > +     mutex_unlock(&cdns->mutex);
> > +     return ret;
> > +}
> > +
> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
> > +{
> > +     enum cdns3_roles role = cdns->role;
> > +
> > +     if (role == CDNS3_ROLE_END)
>
> WARN_ON(role >= CNDS3_ROLE_END) ?
>
> > +             return;
> > +
> > +     mutex_lock(&cdns->mutex);
> > +     cdns->roles[role]->stop(cdns);
> > +     cdns->role = CDNS3_ROLE_END;
>
> Why change the role here? You are just stopping the role not changing it.
> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
> if required without error.
>

The current version of this IP has some issues to detect vbus status correctly,
we have to force vbus status accordingly, so we need a status to indicate
vbus disconnection, and add some code to let controller know vbus
removal, in that case, the controller's state machine can be correct.
So, we increase one role 'CDNS3_ROLE_END' to for this purpose.

CDNS3_ROLE_GADGET: gadget mode and VBUS on
CDNS3_ROLE_HOST: host mode and VBUS on
CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.

So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
and need to set role as CDNS3_ROLE_END at ->stop for further handling at
role switch routine.

> > +     mutex_unlock(&cdns->mutex);
> > +}
> > +
> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> > +{
> > +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> > +             //TODO: implements selecting device/host mode
> > +             return CDNS3_ROLE_HOST;
> > +     }
> > +     return cdns->roles[CDNS3_ROLE_HOST]
> > +             ? CDNS3_ROLE_HOST
> > +             : CDNS3_ROLE_GADGET;
>
> Why not just
>         return cdns->role;
>
> I'm wondering if we really need this function.

cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
If both roles are supported, the role is decided by external
conditions, eg, vbus/id
or external connector. If only single role is supported, only one role structure
is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]

> > +}
>
> > +
> > +/**
> > + * cdns3_core_init_role - initialize role of operation
> > + * @cdns: Pointer to cdns3 structure
> > + *
> > + * Returns 0 on success otherwise negative errno
> > + */
> > +static int cdns3_core_init_role(struct cdns3 *cdns)
> > +{
> > +     struct device *dev = cdns->dev;
> > +     enum usb_dr_mode dr_mode;
> > +
> > +     dr_mode = usb_get_dr_mode(dev);
> > +     cdns->role = CDNS3_ROLE_END;
> > +
> > +     /*
> > +      * If driver can't read mode by means of usb_get_dr_mdoe function then
> > +      * chooses mode according with Kernel configuration. This setting
> > +      * can be restricted later depending on strap pin configuration.
> > +      */
> > +     if (dr_mode == USB_DR_MODE_UNKNOWN) {
> > +             if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> > +                 IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> > +                     dr_mode = USB_DR_MODE_OTG;
> > +             else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> > +                     dr_mode = USB_DR_MODE_HOST;
> > +             else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> > +                     dr_mode = USB_DR_MODE_PERIPHERAL;
> > +     }
> > +
> > +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> > +             //TODO: implements host initialization
>
>                 /* TODO: Add host role */ ?
>
> > +     }
> > +
> > +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
> > +             //TODO: implements device initialization
>
>                 /* TODO: Add device role */ ?
>

Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
cdns->roles[CDNS3_ROLE_GADGET].

> > +     }
> > +
> > +     if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
> > +             dev_err(dev, "no supported roles\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     cdns->dr_mode = dr_mode;

Pawel, why dr_mode needs to be introduced?

> > +     return 0;
> > +}
> > +
> > +/**
> > + * cdns3_irq - interrupt handler for cdns3 core device
> > + *
> > + * @irq: irq number for cdns3 core device
> > + * @data: structure of cdns3
> > + *
> > + * Returns IRQ_HANDLED or IRQ_NONE
> > + */
> > +static irqreturn_t cdns3_irq(int irq, void *data)
> > +{
> > +     struct cdns3 *cdns = data;
> > +     irqreturn_t ret = IRQ_NONE;
> > +
> > +     /* Handle device/host interrupt */
> > +     if (cdns->role != CDNS3_ROLE_END)
>
> Is it because of this that you need to set role to END at role_stop?
> I think it is better to add a state variable to struct cdns3_role_driver, so we can
> check if it is active or stopped.
>
> e.g.
>         if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>
> > +             ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
> > +
> > +     return ret;
> > +}
> > +

 CDNS3_ROLE_END is introduced from above comments, we don't
need another flag for it.
If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt.

> > +static void cdns3_remove_roles(struct cdns3 *cdns)
>
> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>

It is planed to called when at ->remove.
> > +{
> > +     //TODO: implements this function
> > +}
>
> > +
> > +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
> > +{
> > +     enum cdns3_roles current_role;
> > +     int ret = 0;
> > +
> > +     current_role = cdns->role;
> > +
> > +     if (role == CDNS3_ROLE_END)
> > +             return 0;
>
> role == END looks like error state. and it should never happen.
> WARN here?
>

See my comments above.

> > +
> > +     dev_dbg(cdns->dev, "Switching role");
> > +
>
> Don't you have to stop the previous role before starting the new role?
>

Yes, it is needed. Pawel may simply some flows to suit his platform.

> > +     ret = cdns3_role_start(cdns, role);
> > +     if (ret) {
> > +             /* Back to current role */
> > +             dev_err(cdns->dev, "set %d has failed, back to %d\n",
> > +                     role, current_role);
> > +             ret = cdns3_role_start(cdns, current_role);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * cdns3_role_switch - work queue handler for role switch
> > + *
> > + * @work: work queue item structure
> > + *
> > + * Handles below events:
> > + * - Role switch for dual-role devices
> > + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
> > + */
> > +static void cdns3_role_switch(struct work_struct *work)
> > +{
> > +     enum cdns3_roles role = CDNS3_ROLE_END;
> > +     struct cdns3 *cdns;
> > +     bool device, host;
> > +
> > +     cdns = container_of(work, struct cdns3, role_switch_wq);
> > +
> > +     //TODO: implements this functions.
> > +     //host = cdns3_is_host(cdns);
> > +     //device = cdns3_is_device(cdns);
> > +     host = 1;
> > +     device = 0;
> > +
> > +     if (host)
> > +             role = CDNS3_ROLE_HOST;
> > +     else if (device)
> > +             role = CDNS3_ROLE_GADGET;
> > +
> > +     if (cdns->desired_dr_mode == cdns->current_dr_mode &&
> > +         cdns->role == role)
> > +             return;
> > +
>
> I think all the below code can be moved to cdns3_do_role_switch().
>
> > +     pm_runtime_get_sync(cdns->dev);
> > +     cdns3_role_stop(cdns);
> > +
> > +     if (host) {
> > +             if (cdns->roles[CDNS3_ROLE_HOST])
> > +                     cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
> > +             pm_runtime_put_sync(cdns->dev);
> > +             return;
> > +     }
> > +
> > +     if (device)
> > +             cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
> > +     else
> > +             cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
> > +
> > +     pm_runtime_put_sync(cdns->dev);
> > +}
> > +
> > +/**
> > + * cdns3_probe - probe for cdns3 core device
> > + * @pdev: Pointer to cdns3 core platform device
> > + *
> > + * Returns 0 on success otherwise negative errno
> > + */
> > +static int cdns3_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res;
> > +     struct cdns3 *cdns;
> > +     void __iomem *regs;
> > +     int ret;
> > +
> > +     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> > +     if (!cdns)
> > +             return -ENOMEM;
> > +
> > +     cdns->dev = dev;
> > +
> > +     platform_set_drvdata(pdev, cdns);
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +     if (!res) {
> > +             dev_err(dev, "missing IRQ\n");
> > +             return -ENODEV;
> > +     }
> > +     cdns->irq = res->start;
> > +
> > +     /*
> > +      * Request memory region
> > +      * region-0: xHCI
> > +      * region-1: Peripheral
> > +      * region-2: OTG registers
> > +      */
>
> The memory region order is different from the dt-binding.
> There it is OTG, host(xhci), device (peripheral).
>
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     regs = devm_ioremap_resource(dev, res);
> > +
> > +     if (IS_ERR(regs))
> > +             return PTR_ERR(regs);
> > +     cdns->xhci_regs = regs;
> > +     cdns->xhci_res = res;
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +     regs = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(regs))
> > +             return PTR_ERR(regs);
> > +     cdns->dev_regs  = regs;
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > +     regs = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(regs))
> > +             return PTR_ERR(regs);
> > +     cdns->otg_regs = regs;
> > +
> > +     mutex_init(&cdns->mutex);
> > +
> > +     cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>
> "cdns3,usbphy" is not documented in dt-binding.
>
> > +     if (IS_ERR(cdns->phy)) {
> > +             dev_info(dev, "no generic phy found\n");
> > +             cdns->phy = NULL;
> > +             /*
> > +              * fall through here!
> > +              * if no generic phy found, phy init
> > +              * should be done under boot!
> > +              */
>
> No you shouldn't fall through always if it is an error condition.
> Something like this should work better.
>
>         if (IS_ERR(cnds->phy)) {
>                 ret = PTR_ERR(cdns->phy);
>                 if (ret == -ENOSYS || ret == -ENODEV) {
>                         cdns->phy = NULL;
>                 } else if (ret == -EPROBE_DEFER) {
>                         return ret;
>                 } else {
>                         dev_err(dev, "no phy found\n");
>                         goto err0;
>                 }
>         }
>
> So if PHY was provided in DT, and PHY support/drivers is present
> and error condition means something is wrong and we have to error out.
>
> > +     } else {
> > +             phy_init(cdns->phy);
> > +     }
>
> You can do phy_init() outside the else.
>
> > +
> > +     ret = cdns3_core_init_role(cdns);
> > +     if (ret)
> > +             goto err1;
> > +
> > +     INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
> > +     if (ret)
> > +             goto err2;
> > +
> > +     if (ret)
> > +             goto err2;
> > +
> > +     cdns->role = cdns3_get_role(cdns);
>
> I think this should move to cdns3_core_init_role().
>

I agree.

> > +
> > +     ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
> > +                            dev_name(dev), cdns);
> > +
> > +     if (ret)
> > +             goto err2;
>
> How about moving request_irq to before cdsn3_core_init_role()?
>
> Then you can move cdns3_role_start() as well to core_init_role().
>

Usually, we request irq after hardware initialization has finished, if not,
there may unexpected interrupt.

Peter

> > +
> > +     ret = cdns3_role_start(cdns, cdns->role);
> > +     if (ret) {
> > +             dev_err(dev, "can't start %s role\n",
> > +                     cdns3_get_current_role_driver(cdns)->name);
> > +             goto err2;
> > +     }
> > +
> > +     device_set_wakeup_capable(dev, true);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     /*
> > +      * The controller needs less time between bus and controller suspend,
> > +      * and we also needs a small delay to avoid frequently entering low
> > +      * power mode.
> > +      */
> > +     pm_runtime_set_autosuspend_delay(dev, 20);
> > +     pm_runtime_mark_last_busy(dev);
> > +     pm_runtime_use_autosuspend(dev);
> > +     dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
> > +
> > +     return 0;
> > +
> > +err2:
> > +     cdns3_remove_roles(cdns);
> > +err1:
>
> phy_exit() ?
>
> > +     return ret;
> > +}
> > +
> > +/**
> > + * cdns3_remove - unbind drd driver and clean up
> > + * @pdev: Pointer to Linux platform device
> > + *
> > + * Returns 0 on success otherwise negative errno
> > + */
> > +static int cdns3_remove(struct platform_device *pdev)
> > +{
> > +     struct cdns3 *cdns = platform_get_drvdata(pdev);
> > +
> > +     pm_runtime_get_sync(&pdev->dev);
> > +     pm_runtime_disable(&pdev->dev);
> > +     pm_runtime_put_noidle(&pdev->dev);
> > +     cdns3_remove_roles(cdns);
>
> phy_exit() ?
>
> > +
> > +     return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id of_cdns3_match[] = {
> > +     { .compatible = "cdns,usb3" },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_cdns3_match);
> > +#endif
> > +
> > +#ifdef CONFIG_PM
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int cdns3_suspend(struct device *dev)
> > +{
> > +     //TODO: implements this function
> > +     return 0;
> > +}
> > +
> > +static int cdns3_resume(struct device *dev)
> > +{
> > +     //TODO: implements this function
> > +     return 0;
> > +}
> > +#endif /* CONFIG_PM_SLEEP */
> > +static int cdns3_runtime_suspend(struct device *dev)
> > +{    //TODO: implements this function
> > +     return 0;
> > +}
> > +
> > +static int cdns3_runtime_resume(struct device *dev)
> > +{
> > +     //TODO: implements this function
> > +     return 0;
> > +}
> > +#endif /* CONFIG_PM */
> > +
> > +static const struct dev_pm_ops cdns3_pm_ops = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
> > +     SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
> > +};
> > +
> > +static struct platform_driver cdns3_driver = {
> > +     .probe          = cdns3_probe,
> > +     .remove         = cdns3_remove,
> > +     .driver         = {
> > +             .name   = "cdns-usb3",
> > +             .of_match_table = of_match_ptr(of_cdns3_match),
> > +             .pm     = &cdns3_pm_ops,
> > +     },
> > +};
> > +
> > +static int __init cdns3_driver_platform_register(void)
> > +{
> > +     return platform_driver_register(&cdns3_driver);
> > +}
> > +module_init(cdns3_driver_platform_register);
> > +
> > +static void __exit cdns3_driver_platform_unregister(void)
> > +{
> > +     platform_driver_unregister(&cdns3_driver);
> > +}
> > +module_exit(cdns3_driver_platform_unregister);
> > +
> > +MODULE_ALIAS("platform:cdns3");
> > +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> > diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> > new file mode 100644
> > index 000000000000..7c8204fe4d3d
> > --- /dev/null
> > +++ b/drivers/usb/cdns3/core.h
> > @@ -0,0 +1,100 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Cadence USBSS DRD Driver.
> > + *
> > + * Copyright (C) 2017 NXP
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Authors: Peter Chen <[e-mail]>
> > + *          Pawel Laszczak <[e-mail]>
> > + */
> > +#include <linux/usb/otg.h>
> > +
> > +#ifndef __LINUX_CDNS3_CORE_H
> > +#define __LINUX_CDNS3_CORE_H
> > +
> > +struct cdns3;
> > +enum cdns3_roles {
> > +     CDNS3_ROLE_HOST = 0,
> > +     CDNS3_ROLE_GADGET,
> > +     CDNS3_ROLE_END,
> > +};
> > +
> > +/**
> > + * struct cdns3_role_driver - host/gadget role driver
> > + * @start: start this role
> > + * @stop: stop this role
> > + * @suspend: suspend callback for this role
> > + * @resume: resume callback for this role
> > + * @irq: irq handler for this role
> > + * @name: role name string (host/gadget)
> > + */
> > +struct cdns3_role_driver {
> > +     int (*start)(struct cdns3 *cdns);
> > +     void (*stop)(struct cdns3 *cdns);
> > +     int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
> > +     int (*resume)(struct cdns3 *cdns, bool hibernated);
> > +     irqreturn_t (*irq)(struct cdns3 *cdns);
> > +     const char *name;
> > +};
> > +
> > +#define CDNS3_NUM_OF_CLKS    5
> > +/**
> > + * struct cdns3 - Representation of Cadence USB3 DRD controller.
> > + * @dev: pointer to Cadence device struct
> > + * @xhci_regs: pointer to base of xhci registers
> > + * @xhci_res: the resource for xhci
> > + * @dev_regs: pointer to base of dev registers
> > + * @otg_regs: pointer to base of otg registers
> > + * @irq: irq number for controller
> > + * @roles: array of supported roles for this controller
> > + * @role: current role
> > + * @host_dev: the child host device pointer for cdns3 core
> > + * @gadget_dev: the child gadget device pointer for cdns3 core
> > + * @usb: phy for this controller
> > + * @role_switch_wq: work queue item for role switch
> > + * @in_lpm: the controller in low power mode
> > + * @wakeup_int: the wakeup interrupt
> > + * @mutex: the mutex for concurrent code at driver
> > + * @dr_mode: supported mode of operation it can be only Host, only Device
> > + *           or OTG mode that allow to switch between Device and Host mode.
> > + *           This field based on hardware configuration and cant't be changed.
>
> But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.
>
> > + * @current_dr_role: current mode of operation when in dual-role mode
> > + * @desired_dr_role: desired mode of operation when in dual-role mode.
> > + *           This value can be changed during runtime.
> > + *           Available options depends on  dr_mode:
> > + *           dr_mode                 |  desired_dr_role and current_dr_role
> > + *           ----------------------------------------------------------------
> > + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
> > + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
> > + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
> > + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
> > + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
>
> Do you need to update the right hand side to reflect ROLEs instead of MODE?
>
> > + *
> > + *           Desired_dr_role can be changed by means of debugfs.
> > + * @root: debugfs root folder pointer
> > + */
> > +struct cdns3 {
> > +     struct device                   *dev;
> > +     void __iomem                    *xhci_regs;
> > +     struct resource                 *xhci_res;
> > +     struct cdns3_usb_regs __iomem   *dev_regs;
> > +     struct cdns3_otg_regs           *otg_regs;
> > +     int irq;
> > +     struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
> > +     enum cdns3_roles                role;
> > +     struct device                   *host_dev;
> > +     struct device                   *gadget_dev;
> > +     struct phy                      *phy;
> > +     struct work_struct              role_switch_wq;
> > +     int                             in_lpm:1;
> > +     int                             wakeup_int:1;
> > +     /* mutext used in workqueue*/
> > +     struct mutex                    mutex;
> > +     enum usb_dr_mode                dr_mode;
> > +     enum usb_dr_mode                current_dr_mode;
> > +     enum usb_dr_mode                desired_dr_mode;
> > +     struct dentry                   *root;
> > +};
> > +
> > +#endif /* __LINUX_CDNS3_CORE_H */
> >
>
> cheers,
> -roger
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Peter Chen Dec. 4, 2018, 9:09 a.m. UTC | #7

> Hi Roger
>
> >On 18/11/18 12:09, Pawel Laszczak wrote:
> >> Patch adds core.c and core.h file that implements initialization
> >> of platform driver and adds function responsible for selecting,
> >> switching and running appropriate Device/Host mode.
> >>
> >> Signed-off-by: Pawel Laszczak <[e-mail]>
> >> ---
> >>  drivers/usb/cdns3/Makefile |   2 +
> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
> >>  3 files changed, 515 insertions(+)
> >>  create mode 100644 drivers/usb/cdns3/core.c
> >>  create mode 100644 drivers/usb/cdns3/core.h
> >>
> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> >> index dcdd62003c6a..02d25b23c5d3 100644
> >> --- a/drivers/usb/cdns3/Makefile
> >> +++ b/drivers/usb/cdns3/Makefile
> >> @@ -1,3 +1,5 @@
> >> +obj-$(CONFIG_USB_CDNS3)                     += cdns3.o
> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)    += cdns3-pci.o
> >>
> >> +cdns3-y                                     := core.o
> >>  cdns3-pci-y                         := cdns3-pci-wrap.o
> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> new file mode 100644
> >> index 000000000000..f9055d4da67f
> >> --- /dev/null
> >> +++ b/drivers/usb/cdns3/core.c
> >> @@ -0,0 +1,413 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Cadence USBSS DRD Driver.
> >> + *
> >> + * Copyright (C) 2018 Cadence.
> >> + *
> >> + * Author: Peter Chen <[e-mail]>
> >> + *         Pawel Laszczak <[e-mail]>
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/io.h>
> >> +#include <linux/pm_runtime.h>
> >> +
> >> +#include "gadget.h"
> >> +#include "core.h"
> >> +
> >> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> >> +{
> >> +    WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> >> +    return cdns->roles[cdns->role];
> >> +}
> >> +
> >> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
> >> +{
> >> +    int ret;
> >> +
> >> +    if (role >= CDNS3_ROLE_END)
> >
> >WARN_ON()?
> I agree.
> >
> >> +            return 0;
> >> +
> >> +    if (!cdns->roles[role])
> >> +            return -ENXIO;
> >> +
> >> +    mutex_lock(&cdns->mutex);
> >> +    cdns->role = role;
> >> +    ret = cdns->roles[role]->start(cdns);
> >> +    mutex_unlock(&cdns->mutex);
> >> +    return ret;
> >> +}
> >> +
> >> +static inline void cdns3_role_stop(struct cdns3 *cdns)
> >> +{
> >> +    enum cdns3_roles role = cdns->role;
> >> +
> >> +    if (role == CDNS3_ROLE_END)
> >
> >WARN_ON(role >= CNDS3_ROLE_END) ?
> I agree
> >
> >> +            return;
> >> +
> >> +    mutex_lock(&cdns->mutex);
> >> +    cdns->roles[role]->stop(cdns);
> >> +    cdns->role = CDNS3_ROLE_END;
> >
> >Why change the role here? You are just stopping the role not changing it.
> >I think cdns->role should remain unchanged, so we can call cdns3_role_start()
> >if required without error.
>
> This line is unnecessary.
>
> >
> >> +    mutex_unlock(&cdns->mutex);
> >> +}
> >> +
> >> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> >> +{
> >> +    if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> >> +            //TODO: implements selecting device/host mode
> >> +            return CDNS3_ROLE_HOST;
> >> +    }
> >> +    return cdns->roles[CDNS3_ROLE_HOST]
> >> +            ? CDNS3_ROLE_HOST
> >> +            : CDNS3_ROLE_GADGET;
> >
> >Why not just
> >       return cdns->role;
> >
> >I'm wondering if we really need this function
>
> TODO will look likie:
>                 if (cdns3_is_host(cdns))
>                         return CDNS3_ROLE_HOST;
>                 if (cdns3_is_device(cdns))
>                         return CDNS3_ROLE_GADGET;
>
> Function selects initial role. Before invoking it the role is unknown.
> I think that function name should be  changed because current name can be misleading.
>
> I will change it to cdns3_get_initial_role.
> .
> >> +}
> >
> >> +
> >> +/**
> >> + * cdns3_core_init_role - initialize role of operation
> >> + * @cdns: Pointer to cdns3 structure
> >> + *
> >> + * Returns 0 on success otherwise negative errno
> >> + */
> >> +static int cdns3_core_init_role(struct cdns3 *cdns)
> >> +{
> >> +    struct device *dev = cdns->dev;
> >> +    enum usb_dr_mode dr_mode;
> >> +
> >> +    dr_mode = usb_get_dr_mode(dev);
> >> +    cdns->role = CDNS3_ROLE_END;
> >> +
> >> +    /*
> >> +     * If driver can't read mode by means of usb_get_dr_mdoe function then
> >> +     * chooses mode according with Kernel configuration. This setting
> >> +     * can be restricted later depending on strap pin configuration.
> >> +     */
> >> +    if (dr_mode == USB_DR_MODE_UNKNOWN) {
> >> +            if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> >> +                IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> >> +                    dr_mode = USB_DR_MODE_OTG;
> >> +            else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> >> +                    dr_mode = USB_DR_MODE_HOST;
> >> +            else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> >> +                    dr_mode = USB_DR_MODE_PERIPHERAL;
> >> +    }
> >> +
> >> +    if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> >> +            //TODO: implements host initialization
> >
> >               /* TODO: Add host role */ ?
> >
> >> +    }
> >> +
> >> +    if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
> >> +            //TODO: implements device initialization
> >
> >               /* TODO: Add device role */ ?
> >
> >> +    }
> >> +
> >> +    if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
> >> +            dev_err(dev, "no supported roles\n");
> >> +            return -ENODEV;
> >> +    }
> >> +
> >> +    cdns->dr_mode = dr_mode;
> >> +    return 0;
> >> +}
> >> +
> >> +/**
> >> + * cdns3_irq - interrupt handler for cdns3 core device
> >> + *
> >> + * @irq: irq number for cdns3 core device
> >> + * @data: structure of cdns3
> >> + *
> >> + * Returns IRQ_HANDLED or IRQ_NONE
> >> + */
> >> +static irqreturn_t cdns3_irq(int irq, void *data)
> >> +{
> >> +    struct cdns3 *cdns = data;
> >> +    irqreturn_t ret = IRQ_NONE;
> >> +
> >> +    /* Handle device/host interrupt */
> >> +    if (cdns->role != CDNS3_ROLE_END)
> >
> >Is it because of this that you need to set role to END at role_stop?
> >I think it is better to add a state variable to struct cdns3_role_driver, so we can
> >check if it is active or stopped.
> >
> >e.g.
> >       if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>
> Ok, I will do it in this way.
> >> +            ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +static void cdns3_remove_roles(struct cdns3 *cdns)
> >
> >Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>
> Sounds better.
> I also change cdns3_host_remove to cdns3_host_exit and
> cdns3_gadget_remove to cdns3_gadget_exit.
> >
> >> +{
> >> +    //TODO: implements this function
> >> +}
> >
> >> +
> >> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
> >> +{
> >> +    enum cdns3_roles current_role;
> >> +    int ret = 0;
> >> +
> >> +    current_role = cdns->role;
> >> +
> >> +    if (role == CDNS3_ROLE_END)
> >> +            return 0;
> >
> >role == END looks like error state. and it should never happen.
> >WARN here?
>
> Ok, will be changed.
> >

Please consider my comments which replied to Roger just now.

> >> +
> >> +    dev_dbg(cdns->dev, "Switching role");
> >> +
> >
> >Don't you have to stop the previous role before starting the new role?
> >
> >> +    ret = cdns3_role_start(cdns, role);
> >> +    if (ret) {
> >> +            /* Back to current role */
> >> +            dev_err(cdns->dev, "set %d has failed, back to %d\n",
> >> +                    role, current_role);
> >> +            ret = cdns3_role_start(cdns, current_role);
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +/**
> >> + * cdns3_role_switch - work queue handler for role switch
> >> + *
> >> + * @work: work queue item structure
> >> + *
> >> + * Handles below events:
> >> + * - Role switch for dual-role devices
> >> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
> >> + */
> >> +static void cdns3_role_switch(struct work_struct *work)
> >> +{
> >> +    enum cdns3_roles role = CDNS3_ROLE_END;
> >> +    struct cdns3 *cdns;
> >> +    bool device, host;
> >> +
> >> +    cdns = container_of(work, struct cdns3, role_switch_wq);
> >> +
> >> +    //TODO: implements this functions.
> >> +    //host = cdns3_is_host(cdns);
> >> +    //device = cdns3_is_device(cdns);
> >> +    host = 1;
> >> +    device = 0;
> >> +
> >> +    if (host)
> >> +            role = CDNS3_ROLE_HOST;
> >> +    else if (device)
> >> +            role = CDNS3_ROLE_GADGET;
> >> +
> >> +    if (cdns->desired_dr_mode == cdns->current_dr_mode &&
> >> +        cdns->role == role)
> >> +            return;
> >> +
> >
> >I think all the below code can be moved to cdns3_do_role_switch().
>
> Yes, I agree with you. cdns3_role_stop  should be in cdns3_do_role_switch.
>
> >> +    pm_runtime_get_sync(cdns->dev);
> >> +    cdns3_role_stop(cdns);
> >> +
> >> +    if (host) {
> >> +            if (cdns->roles[CDNS3_ROLE_HOST])
> >> +                    cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
> >> +            pm_runtime_put_sync(cdns->dev);
> >> +            return;
> >> +    }
> >> +
> >> +    if (device)
> >> +            cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
> >> +    else
> >> +            cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
> >> +
> >> +    pm_runtime_put_sync(cdns->dev);
> >> +}
> >> +
> >> +/**
> >> + * cdns3_probe - probe for cdns3 core device
> >> + * @pdev: Pointer to cdns3 core platform device
> >> + *
> >> + * Returns 0 on success otherwise negative errno
> >> + */
> >> +static int cdns3_probe(struct platform_device *pdev)
> >> +{
> >> +    struct device *dev = &pdev->dev;
> >> +    struct resource *res;
> >> +    struct cdns3 *cdns;
> >> +    void __iomem *regs;
> >> +    int ret;
> >> +
> >> +    cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> >> +    if (!cdns)
> >> +            return -ENOMEM;
> >> +
> >> +    cdns->dev = dev;
> >> +
> >> +    platform_set_drvdata(pdev, cdns);
> >> +
> >> +    res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +    if (!res) {
> >> +            dev_err(dev, "missing IRQ\n");
> >> +            return -ENODEV;
> >> +    }
> >> +    cdns->irq = res->start;
> >> +
> >> +    /*
> >> +     * Request memory region
> >> +     * region-0: xHCI
> >> +     * region-1: Peripheral
> >> +     * region-2: OTG registers
> >> +     */
> >
> >The memory region order is different from the dt-binding.
> >There it is OTG, host(xhci), device (peripheral).
>
> I corrected dt-binding.
> >
> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +    regs = devm_ioremap_resource(dev, res);
> >> +
> >> +    if (IS_ERR(regs))
> >> +            return PTR_ERR(regs);
> >> +    cdns->xhci_regs = regs;
> >> +    cdns->xhci_res = res;
> >> +
> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +    regs = devm_ioremap_resource(dev, res);
> >> +    if (IS_ERR(regs))
> >> +            return PTR_ERR(regs);
> >> +    cdns->dev_regs  = regs;
> >> +
> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> >> +    regs = devm_ioremap_resource(dev, res);
> >> +    if (IS_ERR(regs))
> >> +            return PTR_ERR(regs);
> >> +    cdns->otg_regs = regs;
> >> +
> >> +    mutex_init(&cdns->mutex);
> >> +
> >> +    cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> >
> >"cdns3,usbphy" is not documented in dt-binding.
>
> I assume that I should add  to dt-binding (cdns3-usb.txt) something like:
>  - phys: reference to the USB PHY
>  - phy-names: name of the USB PHY, should be " cdns3,usbphy "
>
> >
> >> +    if (IS_ERR(cdns->phy)) {
> >> +            dev_info(dev, "no generic phy found\n");
> >> +            cdns->phy = NULL;
> >> +            /*
> >> +             * fall through here!
> >> +             * if no generic phy found, phy init
> >> +             * should be done under boot!
> >> +             */
> >
> >No you shouldn't fall through always if it is an error condition.
> >Something like this should work better.
> >
> >        if (IS_ERR(cnds->phy)) {
> >                ret = PTR_ERR(cdns->phy);
> >                if (ret == -ENOSYS || ret == -ENODEV) {
> >                        cdns->phy = NULL;
> >                } else if (ret == -EPROBE_DEFER) {
> >                        return ret;
> >                } else {
> >                        dev_err(dev, "no phy found\n");
> >                        goto err0;
> >                }
> >        }
> >
> >So if PHY was provided in DT, and PHY support/drivers is present
> >and error condition means something is wrong and we have to error out.
> >
> >> +    } else {
> >> +            phy_init(cdns->phy);
> >> +    }
> >
> >You can do phy_init() outside the else.
> >
> Thank you for explanation. I will correct this.
> >> +
> >> +    ret = cdns3_core_init_role(cdns);
> >> +    if (ret)
> >> +            goto err1;
> >> +
> >> +    INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
> >> +    if (ret)
> >> +            goto err2;
> >> +
> >> +    if (ret)
> >> +            goto err2;
> >> +
> >> +    cdns->role = cdns3_get_role(cdns);
> >
> >I think this should move to cd I'll have a some though on ns3_core_init_role().
>
> Ok, I will do it.
> >
> >> +
> >> +    ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
> >> +                           dev_name(dev), cdns);
> >> +
> >> +    if (ret)
> >> +            goto err2;
> >
> >How about moving request_irq to before cdsn3_core_init_role()?
> >
> >Then you can move cdns3_role_start() as well to core_init_role().
> I'll give it  some though on it, but probably I will probably have to change little other function..
> So the new order should look like this:
>
> cdns3_drd_init
> devm_request_irq
> cdns3_core_init_role, cdns3_get_role, cdns3_role_start
>
> >
> >> +
> >> +    ret = cdns3_role_start(cdns, cdns->role);
> >> +    if (ret) {
> >> +            dev_err(dev, "can't start %s role\n",
> >> +                    cdns3_get_current_role_driver(cdns)->name);
> >> +            goto err2;
> >> +    }
> >> +
> >> +    device_set_wakeup_capable(dev, true);
> >> +    pm_runtime_set_active(dev);
> >> +    pm_runtime_enable(dev);
> >> +
> >> +    /*
> >> +     * The controller needs less time between bus and controller suspend,
> >> +     * and we also needs a small delay to avoid frequently entering low
> >> +     * power mode.
> >> +     */
> >> +    pm_runtime_set_autosuspend_delay(dev, 20);
> >> +    pm_runtime_mark_last_busy(dev);
> >> +    pm_runtime_use_autosuspend(dev);
> >> +    dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
> >> +
> >> +    return 0;
> >> +
> >> +err2:
> >> +    cdns3_remove_roles(cdns);
> >> +err1:
> >
> >phy_exit() ?
> I will add.
> >
> >> +    return ret;
> >> +}
> >> +
> >> +/**
> >> + * cdns3_remove - unbind drd driver and clean up
> >> + * @pdev: Pointer to Linux platform device
> >> + *
> >> + * Returns 0 on success otherwise negative errno
> >> + */
> >> +static int cdns3_remove(struct platform_device *pdev)
> >> +{
> >> +    struct cdns3 *cdns = platform_get_drvdata(pdev);
> >> +
> >> +    pm_runtime_get_sync(&pdev->dev);
> >> +    pm_runtime_disable(&pdev->dev);
> >> +    pm_runtime_put_noidle(&pdev->dev);
> >> +    cdns3_remove_roles(cdns);
> >
> >phy_exit() ?
> I will add.
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id of_cdns3_match[] = {
> >> +    { .compatible = "cdns,usb3" },
> >> +    { },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
> >> +#endif
> >> +
> >> +#ifdef CONFIG_PM
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int cdns3_suspend(struct device *dev)
> >> +{
> >> +    //TODO: implements this function
> >> +    return 0;
> >> +}
> >> +
> >> +static int cdns3_resume(struct device *dev)
> >> +{
> >> +    //TODO: implements this function
> >> +    return 0;
> >> +}
> >> +#endif /* CONFIG_PM_SLEEP */
> >> +static int cdns3_runtime_suspend(struct device *dev)
> >> +{   //TODO: implements this function
> >> +    return 0;
> >> +}
> >> +
> >> +static int cdns3_runtime_resume(struct device *dev)
> >> +{
> >> +    //TODO: implements this function
> >> +    return 0;
> >> +}
> >> +#endif /* CONFIG_PM */
> >> +
> >> +static const struct dev_pm_ops cdns3_pm_ops = {
> >> +    SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
> >> +    SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
> >> +};
> >> +
> >> +static struct platform_driver cdns3_driver = {
> >> +    .probe          = cdns3_probe,
> >> +    .remove         = cdns3_remove,
> >> +    .driver         = {
> >> +            .name   = "cdns-usb3",
> >> +            .of_match_table = of_match_ptr(of_cdns3_match),
> >> +            .pm     = &cdns3_pm_ops,
> >> +    },
> >> +};
> >> +
> >> +static int __init cdns3_driver_platform_register(void)
> >> +{
> >> +    return platform_driver_register(&cdns3_driver);
> >> +}
> >> +module_init(cdns3_driver_platform_register);
> >> +
> >> +static void __exit cdns3_driver_platform_unregister(void)
> >> +{
> >> +    platform_driver_unregister(&cdns3_driver);
> >> +}
> >> +module_exit(cdns3_driver_platform_unregister);
> >> +
> >> +MODULE_ALIAS("platform:cdns3");
> >> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> >> new file mode 100644
> >> index 000000000000..7c8204fe4d3d
> >> --- /dev/null
> >> +++ b/drivers/usb/cdns3/core.h
> >> @@ -0,0 +1,100 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Cadence USBSS DRD Driver.
> >> + *
> >> + * Copyright (C) 2017 NXP
> >> + * Copyright (C) 2018 Cadence.
> >> + *
> >> + * Authors: Peter Chen <[e-mail]>
> >> + *          Pawel Laszczak <[e-mail]>
> >> + */
> >> +#include <linux/usb/otg.h>
> >> +
> >> +#ifndef __LINUX_CDNS3_CORE_H
> >> +#define __LINUX_CDNS3_CORE_H
> >> +
> >> +struct cdns3;
> >> +enum cdns3_roles {
> >> +    CDNS3_ROLE_HOST = 0,
> >> +    CDNS3_ROLE_GADGET,
> >> +    CDNS3_ROLE_END,
> >> +};
> >> +
> >> +/**
> >> + * struct cdns3_role_driver - host/gadget role driver
> >> + * @start: start this role
> >> + * @stop: stop this role
> >> + * @suspend: suspend callback for this role
> >> + * @resume: resume callback for this role
> >> + * @irq: irq handler for this role
> >> + * @name: role name string (host/gadget)
> >> + */
> >> +struct cdns3_role_driver {
> >> +    int (*start)(struct cdns3 *cdns);
> >> +    void (*stop)(struct cdns3 *cdns);
> >> +    int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
> >> +    int (*resume)(struct cdns3 *cdns, bool hibernated);
> >> +    irqreturn_t (*irq)(struct cdns3 *cdns);
> >> +    const char *name;
> >> +};
> >> +
> >> +#define CDNS3_NUM_OF_CLKS   5
> >> +/**
> >> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
> >> + * @dev: pointer to Cadence device struct
> >> + * @xhci_regs: pointer to base of xhci registers
> >> + * @xhci_res: the resource for xhci
> >> + * @dev_regs: pointer to base of dev registers
> >> + * @otg_regs: pointer to base of otg registers
> >> + * @irq: irq number for controller
> >> + * @roles: array of supported roles for this controller
> >> + * @role: current role
> >> + * @host_dev: the child host device pointer for cdns3 core
> >> + * @gadget_dev: the child gadget device pointer for cdns3 core
> >> + * @usb: phy for this controller
> >> + * @role_switch_wq: work queue item for role switch
> >> + * @in_lpm: the controller in low power mode
> >> + * @wakeup_int: the wakeup interrupt
> >> + * @mutex: the mutex for concurrent code at driver
> >> + * @dr_mode: supported mode of operation it can be only Host, only Device
> >> + *           or OTG mode that allow to switch between Device and Host mode.
> >> + *           This field based on hardware configuration and cant't be changed.
> >
> >But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.
> Right, I added dr_mode to dt-binding , so we have STRAP bits in registers and additionally
> optional dr_mode in device-tree. Driver should take into account this two options.
> I will remove this line.
>
> >
> >> + * @current_dr_role: current mode of operation when in dual-role mode
> >> + * @desired_dr_role: desired mode of operation when in dual-role mode.
> >> + *           This value can be changed during runtime.
> >> + *           Available options depends on  dr_mode:
> >> + *           dr_mode                 |  desired_dr_role and current_dr_role
> >> + *           ----------------------------------------------------------------
> >> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
> >> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
> >> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
> >> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
> >> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
> >
> >Do you need to update the right hand side to reflect ROLEs instead of MODE?
>
> I see that there are incorrect name. There should be mode instead role.
> In structure below the names are correct.
> >

Usually, we have two type of roles: init role and current role
init role is decided by dr_mode from firmware, hardware setting and
kernel configuration together.
It is decided at ->probe, if above three settings haveconflict, we
need show an error.
current role is decided at run time, and only used at dual-role mode.
For peripheral-only and host-only application, the current role equals
to init role.

Peter

> >> + *
> >> + *           Desired_dr_role can be changed by means of debugfs.
> >> + * @root: debugfs root folder pointer
> >> + */
> >> +struct cdns3 {
> >> +    struct device                   *dev;
> >> +    void __iomem                    *xhci_regs;
> >> +    struct resource                 *xhci_res;
> >> +    struct cdns3_usb_regs __iomem   *dev_regs;
> >> +    struct cdns3_otg_regs           *otg_regs;
> >> +    int irq;
> >> +    struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
> >> +    enum cdns3_roles                role;
> >> +    struct device                   *host_dev;
> >> +    struct device                   *gadget_dev;
> >> +    struct phy                      *phy;
> >> +    struct work_struct              role_switch_wq;
> >> +    int                             in_lpm:1;
> >> +    int                             wakeup_int:1;
> >> +    /* mutext used in workqueue*/
> >> +    struct mutex                    mutex;
> >> +    enum usb_dr_mode                dr_mode;
> >> +    enum usb_dr_mode                current_dr_mode;
> >> +    enum usb_dr_mode                desired_dr_mode;
> >> +    struct dentry                   *root;
> >> +};
> >> +
> >> +#endif /* __LINUX_CDNS3_CORE_H */
> >>
> >
> >cheers,
> >-roger
> >--
> >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> Thank for all your comments,
> Cheers,
> Pawel

Roger Quadros Dec. 4, 2018, 10:46 a.m. UTC | #8

On 04/12/18 10:50, Peter Chen wrote:
>>> + * Cadence USBSS DRD Driver.
>>> + *
>>> + * Copyright (C) 2018 Cadence.
>>> + *
>>> + * Author: Peter Chen <[e-mail]>
>>> + *         Pawel Laszczak <[e-mail]>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/pm_runtime.h>
>>> +
>>> +#include "gadget.h"
>>> +#include "core.h"
>>> +
>>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>> +{
>>> +     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>>> +     return cdns->roles[cdns->role];
>>> +}
>>> +
>>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>>> +{
>>> +     int ret;
>>> +
>>> +     if (role >= CDNS3_ROLE_END)
>>
>> WARN_ON()?
>>
>>> +             return 0;
>>> +
>>> +     if (!cdns->roles[role])
>>> +             return -ENXIO;
>>> +
>>> +     mutex_lock(&cdns->mutex);
>>> +     cdns->role = role;
>>> +     ret = cdns->roles[role]->start(cdns);
>>> +     mutex_unlock(&cdns->mutex);
>>> +     return ret;
>>> +}
>>> +
>>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>>> +{
>>> +     enum cdns3_roles role = cdns->role;
>>> +
>>> +     if (role == CDNS3_ROLE_END)
>>
>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>
>>> +             return;
>>> +
>>> +     mutex_lock(&cdns->mutex);
>>> +     cdns->roles[role]->stop(cdns);
>>> +     cdns->role = CDNS3_ROLE_END;
>>
>> Why change the role here? You are just stopping the role not changing it.
>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> if required without error.
>>
> 
> The current version of this IP has some issues to detect vbus status correctly,
> we have to force vbus status accordingly, so we need a status to indicate
> vbus disconnection, and add some code to let controller know vbus
> removal, in that case, the controller's state machine can be correct.
> So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
> 
> CDNS3_ROLE_GADGET: gadget mode and VBUS on
> CDNS3_ROLE_HOST: host mode and VBUS on
> CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
> 
> So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
> and need to set role as CDNS3_ROLE_END at ->stop for further handling at
> role switch routine.

OK. but still this (changing to ROLE_END) must be moved to the role switch routine
and the explanation you just mentioned must be added there.

> 
>>> +     mutex_unlock(&cdns->mutex);
>>> +}
>>> +
>>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>> +{
>>> +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>>> +             //TODO: implements selecting device/host mode
>>> +             return CDNS3_ROLE_HOST;
>>> +     }
>>> +     return cdns->roles[CDNS3_ROLE_HOST]
>>> +             ? CDNS3_ROLE_HOST
>>> +             : CDNS3_ROLE_GADGET;
>>
>> Why not just
>>         return cdns->role;
>>
>> I'm wondering if we really need this function.
> 
> cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
> If both roles are supported, the role is decided by external
> conditions, eg, vbus/id
> or external connector. If only single role is supported, only one role structure
> is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
> 

How about adding this description in function documentation.

>>> +}
>>
>>> +
>>> +/**
>>> + * cdns3_core_init_role - initialize role of operation
>>> + * @cdns: Pointer to cdns3 structure
>>> + *
>>> + * Returns 0 on success otherwise negative errno
>>> + */
>>> +static int cdns3_core_init_role(struct cdns3 *cdns)
>>> +{
>>> +     struct device *dev = cdns->dev;
>>> +     enum usb_dr_mode dr_mode;
>>> +
>>> +     dr_mode = usb_get_dr_mode(dev);
>>> +     cdns->role = CDNS3_ROLE_END;
>>> +
>>> +     /*
>>> +      * If driver can't read mode by means of usb_get_dr_mdoe function then
>>> +      * chooses mode according with Kernel configuration. This setting
>>> +      * can be restricted later depending on strap pin configuration.
>>> +      */
>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN) {
>>> +             if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>>> +                 IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>> +                     dr_mode = USB_DR_MODE_OTG;
>>> +             else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>>> +                     dr_mode = USB_DR_MODE_HOST;
>>> +             else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>> +                     dr_mode = USB_DR_MODE_PERIPHERAL;
>>> +     }
>>> +
>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>> +             //TODO: implements host initialization
>>
>>                 /* TODO: Add host role */ ?
>>
>>> +     }
>>> +
>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>>> +             //TODO: implements device initialization
>>
>>                 /* TODO: Add device role */ ?
>>
> 
> Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
> cdns->roles[CDNS3_ROLE_GADGET].
> 
>>> +     }
>>> +
>>> +     if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>>> +             dev_err(dev, "no supported roles\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     cdns->dr_mode = dr_mode;
> 
> Pawel, why dr_mode needs to be introduced?
> 
>>> +     return 0;
>>> +}
>>> +
>>> +/**
>>> + * cdns3_irq - interrupt handler for cdns3 core device
>>> + *
>>> + * @irq: irq number for cdns3 core device
>>> + * @data: structure of cdns3
>>> + *
>>> + * Returns IRQ_HANDLED or IRQ_NONE
>>> + */
>>> +static irqreturn_t cdns3_irq(int irq, void *data)
>>> +{
>>> +     struct cdns3 *cdns = data;
>>> +     irqreturn_t ret = IRQ_NONE;
>>> +
>>> +     /* Handle device/host interrupt */
>>> +     if (cdns->role != CDNS3_ROLE_END)
>>
>> Is it because of this that you need to set role to END at role_stop?
>> I think it is better to add a state variable to struct cdns3_role_driver, so we can
>> check if it is active or stopped.
>>
>> e.g.
>>         if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>>
>>> +             ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>>> +
>>> +     return ret;
>>> +}
>>> +
> 
>  CDNS3_ROLE_END is introduced from above comments, we don't
> need another flag for it.
> If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt.
> 
>>> +static void cdns3_remove_roles(struct cdns3 *cdns)
>>
>> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>>
> 
> It is planed to called when at ->remove.
>>> +{
>>> +     //TODO: implements this function
>>> +}
>>
>>> +
>>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>>> +{
>>> +     enum cdns3_roles current_role;
>>> +     int ret = 0;
>>> +
>>> +     current_role = cdns->role;
>>> +
>>> +     if (role == CDNS3_ROLE_END)
>>> +             return 0;
>>
>> role == END looks like error state. and it should never happen.
>> WARN here?
>>
> 
> See my comments above.
> 
>>> +
>>> +     dev_dbg(cdns->dev, "Switching role");
>>> +
>>
>> Don't you have to stop the previous role before starting the new role?
>>
> 
> Yes, it is needed. Pawel may simply some flows to suit his platform.
> 
>>> +     ret = cdns3_role_start(cdns, role);
>>> +     if (ret) {
>>> +             /* Back to current role */
>>> +             dev_err(cdns->dev, "set %d has failed, back to %d\n",
>>> +                     role, current_role);
>>> +             ret = cdns3_role_start(cdns, current_role);
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +/**
>>> + * cdns3_role_switch - work queue handler for role switch
>>> + *
>>> + * @work: work queue item structure
>>> + *
>>> + * Handles below events:
>>> + * - Role switch for dual-role devices
>>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>>> + */
>>> +static void cdns3_role_switch(struct work_struct *work)
>>> +{
>>> +     enum cdns3_roles role = CDNS3_ROLE_END;
>>> +     struct cdns3 *cdns;
>>> +     bool device, host;
>>> +
>>> +     cdns = container_of(work, struct cdns3, role_switch_wq);
>>> +
>>> +     //TODO: implements this functions.
>>> +     //host = cdns3_is_host(cdns);
>>> +     //device = cdns3_is_device(cdns);
>>> +     host = 1;
>>> +     device = 0;
>>> +
>>> +     if (host)
>>> +             role = CDNS3_ROLE_HOST;
>>> +     else if (device)
>>> +             role = CDNS3_ROLE_GADGET;
>>> +
>>> +     if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>>> +         cdns->role == role)
>>> +             return;
>>> +
>>
>> I think all the below code can be moved to cdns3_do_role_switch().
>>
>>> +     pm_runtime_get_sync(cdns->dev);
>>> +     cdns3_role_stop(cdns);
>>> +
>>> +     if (host) {
>>> +             if (cdns->roles[CDNS3_ROLE_HOST])
>>> +                     cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>>> +             pm_runtime_put_sync(cdns->dev);
>>> +             return;
>>> +     }
>>> +
>>> +     if (device)
>>> +             cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>>> +     else
>>> +             cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>>> +
>>> +     pm_runtime_put_sync(cdns->dev);
>>> +}
>>> +
>>> +/**
>>> + * cdns3_probe - probe for cdns3 core device
>>> + * @pdev: Pointer to cdns3 core platform device
>>> + *
>>> + * Returns 0 on success otherwise negative errno
>>> + */
>>> +static int cdns3_probe(struct platform_device *pdev)
>>> +{
>>> +     struct device *dev = &pdev->dev;
>>> +     struct resource *res;
>>> +     struct cdns3 *cdns;
>>> +     void __iomem *regs;
>>> +     int ret;
>>> +
>>> +     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>>> +     if (!cdns)
>>> +             return -ENOMEM;
>>> +
>>> +     cdns->dev = dev;
>>> +
>>> +     platform_set_drvdata(pdev, cdns);
>>> +
>>> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> +     if (!res) {
>>> +             dev_err(dev, "missing IRQ\n");
>>> +             return -ENODEV;
>>> +     }
>>> +     cdns->irq = res->start;
>>> +
>>> +     /*
>>> +      * Request memory region
>>> +      * region-0: xHCI
>>> +      * region-1: Peripheral
>>> +      * region-2: OTG registers
>>> +      */
>>
>> The memory region order is different from the dt-binding.
>> There it is OTG, host(xhci), device (peripheral).
>>
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +     regs = devm_ioremap_resource(dev, res);
>>> +
>>> +     if (IS_ERR(regs))
>>> +             return PTR_ERR(regs);
>>> +     cdns->xhci_regs = regs;
>>> +     cdns->xhci_res = res;
>>> +
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +     regs = devm_ioremap_resource(dev, res);
>>> +     if (IS_ERR(regs))
>>> +             return PTR_ERR(regs);
>>> +     cdns->dev_regs  = regs;
>>> +
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>> +     regs = devm_ioremap_resource(dev, res);
>>> +     if (IS_ERR(regs))
>>> +             return PTR_ERR(regs);
>>> +     cdns->otg_regs = regs;
>>> +
>>> +     mutex_init(&cdns->mutex);
>>> +
>>> +     cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>>
>> "cdns3,usbphy" is not documented in dt-binding.
>>
>>> +     if (IS_ERR(cdns->phy)) {
>>> +             dev_info(dev, "no generic phy found\n");
>>> +             cdns->phy = NULL;
>>> +             /*
>>> +              * fall through here!
>>> +              * if no generic phy found, phy init
>>> +              * should be done under boot!
>>> +              */
>>
>> No you shouldn't fall through always if it is an error condition.
>> Something like this should work better.
>>
>>         if (IS_ERR(cnds->phy)) {
>>                 ret = PTR_ERR(cdns->phy);
>>                 if (ret == -ENOSYS || ret == -ENODEV) {
>>                         cdns->phy = NULL;
>>                 } else if (ret == -EPROBE_DEFER) {
>>                         return ret;
>>                 } else {
>>                         dev_err(dev, "no phy found\n");
>>                         goto err0;
>>                 }
>>         }
>>
>> So if PHY was provided in DT, and PHY support/drivers is present
>> and error condition means something is wrong and we have to error out.
>>
>>> +     } else {
>>> +             phy_init(cdns->phy);
>>> +     }
>>
>> You can do phy_init() outside the else.
>>
>>> +
>>> +     ret = cdns3_core_init_role(cdns);
>>> +     if (ret)
>>> +             goto err1;
>>> +
>>> +     INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>>> +     if (ret)
>>> +             goto err2;
>>> +
>>> +     if (ret)
>>> +             goto err2;
>>> +
>>> +     cdns->role = cdns3_get_role(cdns);
>>
>> I think this should move to cdns3_core_init_role().
>>
> 
> I agree.
> 
>>> +
>>> +     ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>>> +                            dev_name(dev), cdns);
>>> +
>>> +     if (ret)
>>> +             goto err2;
>>
>> How about moving request_irq to before cdsn3_core_init_role()?
>>
>> Then you can move cdns3_role_start() as well to core_init_role().
>>
> 
> Usually, we request irq after hardware initialization has finished, if not,
> there may unexpected interrupt.

Doesn't kernel warn if interrupt happens and there is no handler?
To avoid that I was suggesting to request_irq first.

cheers,
-roger

Pawel Laszczak Dec. 5, 2018, 7:19 a.m. UTC | #9

Hi,

>> >>
>> >> Patch adds core.c and core.h file that implements initialization
>> >> of platform driver and adds function responsible for selecting,
>> >> switching and running appropriate Device/Host mode.
>> >>
>> >> Signed-off-by: Pawel Laszczak <[e-mail]>
>> >> ---
>> >>  drivers/usb/cdns3/Makefile |   2 +
>> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
>> >>  3 files changed, 515 insertions(+)
>> >>  create mode 100644 drivers/usb/cdns3/core.c
>> >>  create mode 100644 drivers/usb/cdns3/core.h
>> >>
>> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> >> index dcdd62003c6a..02d25b23c5d3 100644
>> >> --- a/drivers/usb/cdns3/Makefile
>> >> +++ b/drivers/usb/cdns3/Makefile
>> >> @@ -1,3 +1,5 @@
>> >> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
>> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
>> >>
>> >> +cdns3-y                                        := core.o
>> >>  cdns3-pci-y                            := cdns3-pci-wrap.o
>> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> >> new file mode 100644
>> >> index 000000000000..f9055d4da67f
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.c
>> >> @@ -0,0 +1,413 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >> + * Copyright (C) 2018 Cadence.
>> >> + *
>> >
>> >Please add NXP copyright too.
>>
>> Ok, I don't know why I omitted this.
>> I know that you are the main author of this file
>> Sorry for that.
>>
>> One additional question. What year I should add in Copyright for NXP?.
>> The original year 2017 or I should modified all to 2018.
>>
>Please use below copyright, thanks.
>
>Copyright 2017-2018 NXP

I add this in all files  modified or created by you.
>
>
>
>> >> +       mutex_init(&cdns->mutex);
>> >> +
>> >> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>> >> +       if (IS_ERR(cdns->phy)) {
>> >> +               dev_info(dev, "no generic phy found\n");
>> >> +               cdns->phy = NULL;
>> >> +               /*
>> >> +                * fall through here!
>> >> +                * if no generic phy found, phy init
>> >> +                * should be done under boot!
>> >> +                */
>> >
>> >If the phy driver is defer-probed, it will be here, it is not an error.
>> >I think you could have a generic phy driver or usb generic phy driver
>> >(drivers/usb/phy/phy-generic.c) even you don't need any operations for
>> >PHY. It will be easy for other platforms.
>>
>> Yes, Roger ask me to modify this fragment. In next version it will look like:
>>         cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>>         if (IS_ERR(cdns->phy)) {
>>                 ret = PTR_ERR(cdns->phy);
>>                 if (ret == -ENOSYS || ret == -ENODEV) {
>>                         cdns->phy = NULL;
>>                 } else if (ret == -EPROBE_DEFER) {
>>                         return ret;
>>                 } else {
>>                         dev_err(dev, "no phy found\n");
>>                         goto err0;
>>                 }
>>         }
>>
>>         phy_init(cdns->phy);
>>
>> We are going to use phy driver. I don't know if it correct.
>> I don't have experience in this filed.
>> We need phy initialization but I don't have testing platform now.
>> In most usb drivers I see that there are used usb phy driverd instead phy dirverd.
>>
>
>At my CDNS3 platform, there are some USB PHY initialization for register setting
>and clock enable. You could add generic PHY driver under: drivers/phy/cadence/.
>
>Above PHY initialization code is OK for me.

It will be added as separate driver. 
I think that Allan Douglas working on it. 
I ask him to add you to -cc in patch for phy.

>
>
>> >> +static void __exit cdns3_driver_platform_unregister(void)
>> >> +{
>> >> +       platform_driver_unregister(&cdns3_driver);
>> >> +}
>> >> +module_exit(cdns3_driver_platform_unregister);
>> >> +
>> >> +MODULE_ALIAS("platform:cdns3");
>> >> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
>> >> +MODULE_LICENSE("GPL v2");
>> >> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> >> new file mode 100644
>> >> index 000000000000..7c8204fe4d3d
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.h
>> >> @@ -0,0 +1,100 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >
>> >Header file
>> I don't understand. What is wrong ?
>> >
>
>The comment for this file
>
>Cadence USBSS DRD Core Header File

Ok, thanks 
Cheers, 
Pawel

Alan Douglas Dec. 5, 2018, 8:55 a.m. UTC | #10

Hi Peter,

On 05 December 2018 07:20, Pawel Laszczak wrote:
> Hi,
> 
> >> >>
> >> >> Patch adds core.c and core.h file that implements initialization
> >> >> of platform driver and adds function responsible for selecting,
> >> >> switching and running appropriate Device/Host mode.
> >> >>
> >> >> Signed-off-by: Pawel Laszczak <[e-mail]>
> >> >> ---
> >> >>  drivers/usb/cdns3/Makefile |   2 +
> >> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
> >> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
> >> >>  3 files changed, 515 insertions(+)
> >> >>  create mode 100644 drivers/usb/cdns3/core.c
> >> >>  create mode 100644 drivers/usb/cdns3/core.h
> >> >>
> >> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> >> >> index dcdd62003c6a..02d25b23c5d3 100644
> >> >> --- a/drivers/usb/cdns3/Makefile
> >> >> +++ b/drivers/usb/cdns3/Makefile
> >> >> @@ -1,3 +1,5 @@
> >> >> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
> >> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
> >> >>
> >> >> +cdns3-y                                        := core.o
> >> >>  cdns3-pci-y                            := cdns3-pci-wrap.o
> >> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> >> new file mode 100644
> >> >> index 000000000000..f9055d4da67f
> >> >> --- /dev/null
> >> >> +++ b/drivers/usb/cdns3/core.c
> >> >> @@ -0,0 +1,413 @@
> >> >> +// SPDX-License-Identifier: GPL-2.0
> >> >> +/*
> >> >> + * Cadence USBSS DRD Driver.
> >> >> + *
> >> >> + * Copyright (C) 2018 Cadence.
> >> >> + *
> >> >
> >> >Please add NXP copyright too.
> >>
> >> Ok, I don't know why I omitted this.
> >> I know that you are the main author of this file
> >> Sorry for that.
> >>
> >> One additional question. What year I should add in Copyright for NXP?.
> >> The original year 2017 or I should modified all to 2018.
> >>
> >Please use below copyright, thanks.
> >
> >Copyright 2017-2018 NXP
> 
> I add this in all files  modified or created by you.
> >
> >
> >
> >> >> +       mutex_init(&cdns->mutex);
> >> >> +
> >> >> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> >> >> +       if (IS_ERR(cdns->phy)) {
> >> >> +               dev_info(dev, "no generic phy found\n");
> >> >> +               cdns->phy = NULL;
> >> >> +               /*
> >> >> +                * fall through here!
> >> >> +                * if no generic phy found, phy init
> >> >> +                * should be done under boot!
> >> >> +                */
> >> >
> >> >If the phy driver is defer-probed, it will be here, it is not an error.
> >> >I think you could have a generic phy driver or usb generic phy driver
> >> >(drivers/usb/phy/phy-generic.c) even you don't need any operations for
> >> >PHY. It will be easy for other platforms.
> >>
> >> Yes, Roger ask me to modify this fragment. In next version it will look like:
> >>         cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> >>         if (IS_ERR(cdns->phy)) {
> >>                 ret = PTR_ERR(cdns->phy);
> >>                 if (ret == -ENOSYS || ret == -ENODEV) {
> >>                         cdns->phy = NULL;
> >>                 } else if (ret == -EPROBE_DEFER) {
> >>                         return ret;
> >>                 } else {
> >>                         dev_err(dev, "no phy found\n");
> >>                         goto err0;
> >>                 }
> >>         }
> >>
> >>         phy_init(cdns->phy);
> >>
> >> We are going to use phy driver. I don't know if it correct.
> >> I don't have experience in this filed.
> >> We need phy initialization but I don't have testing platform now.
> >> In most usb drivers I see that there are used usb phy driverd instead phy dirverd.
> >>
> >
> >At my CDNS3 platform, there are some USB PHY initialization for register setting
> >and clock enable. You could add generic PHY driver under: drivers/phy/cadence/.
> >
> >Above PHY initialization code is OK for me.
> 
> It will be added as separate driver.
> I think that Allan Douglas working on it.
> I ask him to add you to -cc in patch for phy.
Patch series for the cadence Sierra generic PHY driver can be found here:
https://lore.kernel.org/patchwork/cover/1011486/

It can also be found in Kishon's linux-phy git tree at
    git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git
in the 'next' branch.

It will be great if you are able to take a look at it.

Regards,
Alan

Peter Chen Dec. 5, 2018, 8:57 a.m. UTC | #11

>
>
>
> On 04/12/18 10:50, Peter Chen wrote:
> >>> + * Cadence USBSS DRD Driver.
> >>> + *
> >>> + * Copyright (C) 2018 Cadence.
> >>> + *
> >>> + * Author: Peter Chen <[e-mail]>
> >>> + *         Pawel Laszczak <[e-mail]>
> >>> + */
> >>> +
> >>> +#include <linux/module.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/io.h>
> >>> +#include <linux/pm_runtime.h>
> >>> +
> >>> +#include "gadget.h"
> >>> +#include "core.h"
> >>> +
> >>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
> >>> +{
> >>> +     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> >>> +     return cdns->roles[cdns->role];
> >>> +}
> >>> +
> >>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
> >>> +{
> >>> +     int ret;
> >>> +
> >>> +     if (role >= CDNS3_ROLE_END)
> >>
> >> WARN_ON()?
> >>
> >>> +             return 0;
> >>> +
> >>> +     if (!cdns->roles[role])
> >>> +             return -ENXIO;
> >>> +
> >>> +     mutex_lock(&cdns->mutex);
> >>> +     cdns->role = role;
> >>> +     ret = cdns->roles[role]->start(cdns);
> >>> +     mutex_unlock(&cdns->mutex);
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
> >>> +{
> >>> +     enum cdns3_roles role = cdns->role;
> >>> +
> >>> +     if (role == CDNS3_ROLE_END)
> >>
> >> WARN_ON(role >= CNDS3_ROLE_END) ?
> >>
> >>> +             return;
> >>> +
> >>> +     mutex_lock(&cdns->mutex);
> >>> +     cdns->roles[role]->stop(cdns);
> >>> +     cdns->role = CDNS3_ROLE_END;
> >>
> >> Why change the role here? You are just stopping the role not changing it.
> >> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
> >> if required without error.
> >>
> >
> > The current version of this IP has some issues to detect vbus status correctly,
> > we have to force vbus status accordingly, so we need a status to indicate
> > vbus disconnection, and add some code to let controller know vbus
> > removal, in that case, the controller's state machine can be correct.
> > So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
> >
> > CDNS3_ROLE_GADGET: gadget mode and VBUS on
> > CDNS3_ROLE_HOST: host mode and VBUS on
> > CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
> >
> > So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
> > and need to set role as CDNS3_ROLE_END at ->stop for further handling at
> > role switch routine.
>
> OK. but still this (changing to ROLE_END) must be moved to the role switch routine
> and the explanation you just mentioned must be added there.
>

If we use host part as separate driver, something may need to change.
The ROLE_END
may need to add at role switch routine as your said.

> >
> >>> +     mutex_unlock(&cdns->mutex);
> >>> +}
> >>> +
> >>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> >>> +{
> >>> +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> >>> +             //TODO: implements selecting device/host mode
> >>> +             return CDNS3_ROLE_HOST;
> >>> +     }
> >>> +     return cdns->roles[CDNS3_ROLE_HOST]
> >>> +             ? CDNS3_ROLE_HOST
> >>> +             : CDNS3_ROLE_GADGET;
> >>
> >> Why not just
> >>         return cdns->role;
> >>
> >> I'm wondering if we really need this function.
> >
> > cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
> > If both roles are supported, the role is decided by external
> > conditions, eg, vbus/id
> > or external connector. If only single role is supported, only one role structure
> > is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
> >
>
> How about adding this description in function documentation.
>

Sure, but may need to redesign it due to host part as a standalone driver.


> >>
> >> Then you can move cdns3_role_start() as well to core_init_role().
> >>
> >
> > Usually, we request irq after hardware initialization has finished, if not,
> > there may unexpected interrupt.
>
> Doesn't kernel warn if interrupt happens and there is no handler?
> To avoid that I was suggesting to request_irq first.
>

The interrupt will not happen (related bit at GIC is masked) until we
call request_irq.
And we don't want to handle interrupts until hardware initialization, don't we?

Peter

Peter Chen Dec. 5, 2018, 9:07 a.m. UTC | #12

On Wed, Dec 5, 2018 at 4:55 PM Alan Douglas <[e-mail]> wrote:
>
> Hi Peter,
>
> On 05 December 2018 07:20, Pawel Laszczak wrote:
> > Hi,
> >
> > >> >>
> > >> >> Patch adds core.c and core.h file that implements initialization
> > >> >> of platform driver and adds function responsible for selecting,
> > >> >> switching and running appropriate Device/Host mode.
> > >> >>
> > >> >> Signed-off-by: Pawel Laszczak <[e-mail]>
> > >> >> ---
> > >> >>  drivers/usb/cdns3/Makefile |   2 +
> > >> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
> > >> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
> > >> >>  3 files changed, 515 insertions(+)
> > >> >>  create mode 100644 drivers/usb/cdns3/core.c
> > >> >>  create mode 100644 drivers/usb/cdns3/core.h
> > >> >>
> > >> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> > >> >> index dcdd62003c6a..02d25b23c5d3 100644
> > >> >> --- a/drivers/usb/cdns3/Makefile
> > >> >> +++ b/drivers/usb/cdns3/Makefile
> > >> >> @@ -1,3 +1,5 @@
> > >> >> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
> > >> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
> > >> >>
> > >> >> +cdns3-y                                        := core.o
> > >> >>  cdns3-pci-y                            := cdns3-pci-wrap.o
> > >> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > >> >> new file mode 100644
> > >> >> index 000000000000..f9055d4da67f
> > >> >> --- /dev/null
> > >> >> +++ b/drivers/usb/cdns3/core.c
> > >> >> @@ -0,0 +1,413 @@
> > >> >> +// SPDX-License-Identifier: GPL-2.0
> > >> >> +/*
> > >> >> + * Cadence USBSS DRD Driver.
> > >> >> + *
> > >> >> + * Copyright (C) 2018 Cadence.
> > >> >> + *
> > >> >
> > >> >Please add NXP copyright too.
> > >>
> > >> Ok, I don't know why I omitted this.
> > >> I know that you are the main author of this file
> > >> Sorry for that.
> > >>
> > >> One additional question. What year I should add in Copyright for NXP?.
> > >> The original year 2017 or I should modified all to 2018.
> > >>
> > >Please use below copyright, thanks.
> > >
> > >Copyright 2017-2018 NXP
> >
> > I add this in all files  modified or created by you.
> > >
> > >
> > >
> > >> >> +       mutex_init(&cdns->mutex);
> > >> >> +
> > >> >> +       cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> > >> >> +       if (IS_ERR(cdns->phy)) {
> > >> >> +               dev_info(dev, "no generic phy found\n");
> > >> >> +               cdns->phy = NULL;
> > >> >> +               /*
> > >> >> +                * fall through here!
> > >> >> +                * if no generic phy found, phy init
> > >> >> +                * should be done under boot!
> > >> >> +                */
> > >> >
> > >> >If the phy driver is defer-probed, it will be here, it is not an error.
> > >> >I think you could have a generic phy driver or usb generic phy driver
> > >> >(drivers/usb/phy/phy-generic.c) even you don't need any operations for
> > >> >PHY. It will be easy for other platforms.
> > >>
> > >> Yes, Roger ask me to modify this fragment. In next version it will look like:
> > >>         cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
> > >>         if (IS_ERR(cdns->phy)) {
> > >>                 ret = PTR_ERR(cdns->phy);
> > >>                 if (ret == -ENOSYS || ret == -ENODEV) {
> > >>                         cdns->phy = NULL;
> > >>                 } else if (ret == -EPROBE_DEFER) {
> > >>                         return ret;
> > >>                 } else {
> > >>                         dev_err(dev, "no phy found\n");
> > >>                         goto err0;
> > >>                 }
> > >>         }
> > >>
> > >>         phy_init(cdns->phy);
> > >>
> > >> We are going to use phy driver. I don't know if it correct.
> > >> I don't have experience in this filed.
> > >> We need phy initialization but I don't have testing platform now.
> > >> In most usb drivers I see that there are used usb phy driverd instead phy dirverd.
> > >>
> > >
> > >At my CDNS3 platform, there are some USB PHY initialization for register setting
> > >and clock enable. You could add generic PHY driver under: drivers/phy/cadence/.
> > >
> > >Above PHY initialization code is OK for me.
> >
> > It will be added as separate driver.
> > I think that Allan Douglas working on it.
> > I ask him to add you to -cc in patch for phy.
> Patch series for the cadence Sierra generic PHY driver can be found here:
> https://lore.kernel.org/patchwork/cover/1011486/
>
> It can also be found in Kishon's linux-phy git tree at
>     git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git
> in the 'next' branch.
>
> It will be great if you are able to take a look at it.

I will test it, and submit some changes if needed, thanks.

Peter

Pawel Laszczak Dec. 5, 2018, 7:24 p.m. UTC | #13

Hi

+ Tomek

>> > + * Cadence USBSS DRD Driver.
>> > + *
>> > + * Copyright (C) 2018 Cadence.
>> > + *
>> > + * Author: Peter Chen <[e-mail]>
>> > + *         Pawel Laszczak <[e-mail]>
>> > + */
>> > +
>> > +#include <linux/module.h>
>> > +#include <linux/kernel.h>
>> > +#include <linux/platform_device.h>
>> > +#include <linux/interrupt.h>
>> > +#include <linux/io.h>
>> > +#include <linux/pm_runtime.h>
>> > +
>> > +#include "gadget.h"
>> > +#include "core.h"
>> > +
>> > +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>> > +{
>> > +     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>> > +     return cdns->roles[cdns->role];
>> > +}
>> > +
>> > +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>> > +{
>> > +     int ret;
>> > +
>> > +     if (role >= CDNS3_ROLE_END)
>>
>> WARN_ON()?
>>
>> > +             return 0;
>> > +
>> > +     if (!cdns->roles[role])
>> > +             return -ENXIO;
>> > +
>> > +     mutex_lock(&cdns->mutex);
>> > +     cdns->role = role;
>> > +     ret = cdns->roles[role]->start(cdns);
>> > +     mutex_unlock(&cdns->mutex);
>> > +     return ret;
>> > +}
>> > +
>> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> > +{
>> > +     enum cdns3_roles role = cdns->role;
>> > +
>> > +     if (role == CDNS3_ROLE_END)
>>
>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>
>> > +             return;
>> > +
>> > +     mutex_lock(&cdns->mutex);
>> > +     cdns->roles[role]->stop(cdns);
>> > +     cdns->role = CDNS3_ROLE_END;
>>
>> Why change the role here? You are just stopping the role not changing it.
>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> if required without error.
>>
>
>The current version of this IP has some issues to detect vbus status correctly,
>we have to force vbus status accordingly, so we need a status to indicate
>vbus disconnection, and add some code to let controller know vbus
>removal, in that case, the controller's state machine can be correct.
>So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
>
>CDNS3_ROLE_GADGET: gadget mode and VBUS on
>CDNS3_ROLE_HOST: host mode and VBUS on
>CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.

>
>So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
>and need to set role as CDNS3_ROLE_END at ->stop for further handling at
>role switch routine.
>
>> > +     mutex_unlock(&cdns->mutex);
>> > +}
>> > +
>> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> > +{
>> > +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> > +             //TODO: implements selecting device/host mode
>> > +             return CDNS3_ROLE_HOST;
>> > +     }
>> > +     return cdns->roles[CDNS3_ROLE_HOST]
>> > +             ? CDNS3_ROLE_HOST
>> > +             : CDNS3_ROLE_GADGET;
>>
>> Why not just
>>         return cdns->role;
>>
>> I'm wondering if we really need this function.
>
>cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
>If both roles are supported, the role is decided by external
>conditions, eg, vbus/id
>or external connector. If only single role is supported, only one role structure
>is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
>
>> > +}
>>
>> > +
>> > +/**
>> > + * cdns3_core_init_role - initialize role of operation
>> > + * @cdns: Pointer to cdns3 structure
>> > + *
>> > + * Returns 0 on success otherwise negative errno
>> > + */
>> > +static int cdns3_core_init_role(struct cdns3 *cdns)
>> > +{
>> > +     struct device *dev = cdns->dev;
>> > +     enum usb_dr_mode dr_mode;
>> > +
>> > +     dr_mode = usb_get_dr_mode(dev);
>> > +     cdns->role = CDNS3_ROLE_END;
>> > +
>> > +     /*
>> > +      * If driver can't read mode by means of usb_get_dr_mdoe function then
>> > +      * chooses mode according with Kernel configuration. This setting
>> > +      * can be restricted later depending on strap pin configuration.
>> > +      */
>> > +     if (dr_mode == USB_DR_MODE_UNKNOWN) {
>> > +             if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>> > +                 IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> > +                     dr_mode = USB_DR_MODE_OTG;
>> > +             else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>> > +                     dr_mode = USB_DR_MODE_HOST;
>> > +             else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> > +                     dr_mode = USB_DR_MODE_PERIPHERAL;
>> > +     }
>> > +
>> > +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> > +             //TODO: implements host initialization
>>
>>                 /* TODO: Add host role */ ?
>>
>> > +     }
>> > +
>> > +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>> > +             //TODO: implements device initialization
>>
>>                 /* TODO: Add device role */ ?
>>
>
>Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
>cdns->roles[CDNS3_ROLE_GADGET].
>
>> > +     }
>> > +
>> > +     if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>> > +             dev_err(dev, "no supported roles\n");
>> > +             return -ENODEV;
>> > +     }
>> > +
>> > +     cdns->dr_mode = dr_mode;
>
>Pawel, why dr_mode needs to be introduced?

cdns->dr_mode - it's the mode depending on configuration. 
We have device tree from which we can read dr_mode. It can be Host, Device or OTG - the maximum is OTG.
Additionally we have  strap pin that allow to force selected mode. 
Additionally mode can be limited by kernel configuration. 

So, I assume that controller can support OTG (HOST+DEVICE) , but it can be restricted by kernel configuration, strap pin or DTS (dr_mode property). 

When this configuration is restricted by one of these then if dr_mode is :
Host - we simply load XHCI driver
Device  - we start Device part
OTG - we based on ID pin

Eg. 
Controller support OTG mode but STRAP  is set to Host only then  dr_mode is set to  host
Controller support OTG mode but STRAP  is set to Device only then  dr_mode is set  to device 
Controller support OTG mode and STRAP  is set to OTG then  dr_mode is set  to OTG and  it can support Device only, Host only or OTG mode.

Additionally I'm introduce dr_mode that can be changed from user space. It can use for testing purpose. 
So even dr_mode  = OTG then we can force switching role from user space. 

I'm using it for testing. 

So additionally in cdns3 object we have desired_dr_mode and current_dr_mode. 

>
>> > +     return 0;
>> > +}
>> > +
>> > +/**
>> > + * cdns3_irq - interrupt handler for cdns3 core device
>> > + *
>> > + * @irq: irq number for cdns3 core device
>> > + * @data: structure of cdns3
>> > + *
>> > + * Returns IRQ_HANDLED or IRQ_NONE
>> > + */
>> > +static irqreturn_t cdns3_irq(int irq, void *data)
>> > +{
>> > +     struct cdns3 *cdns = data;
>> > +     irqreturn_t ret = IRQ_NONE;
>> > +
>> > +     /* Handle device/host interrupt */
>> > +     if (cdns->role != CDNS3_ROLE_END)
>>
>> Is it because of this that you need to set role to END at role_stop?
>> I think it is better to add a state variable to struct cdns3_role_driver, so we can
>> check if it is active or stopped.
>>
>> e.g.
>>         if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>>
>> > +             ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>> > +
>> > +     return ret;
>> > +}
>> > +
>
> CDNS3_ROLE_END is introduced from above comments, we don't
>need another flag for it.
>If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt.
>
>> > +static void cdns3_remove_roles(struct cdns3 *cdns)
>>
>> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>>
>
>It is planed to called when at ->remove.
>> > +{
>> > +     //TODO: implements this function
>> > +}
>>
>> > +
>> > +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>> > +{
>> > +     enum cdns3_roles current_role;
>> > +     int ret = 0;
>> > +
>> > +     current_role = cdns->role;
>> > +
>> > +     if (role == CDNS3_ROLE_END)
>> > +             return 0;
>>
>> role == END looks like error state. and it should never happen.
>> WARN here?
>>
>
>See my comments above.
>
>> > +
>> > +     dev_dbg(cdns->dev, "Switching role");
>> > +
>>
>> Don't you have to stop the previous role before starting the new role?
>>
>
>Yes, it is needed. Pawel may simply some flows to suit his platform.

>
>> > +     ret = cdns3_role_start(cdns, role);
>> > +     if (ret) {
>> > +             /* Back to current role */
>> > +             dev_err(cdns->dev, "set %d has failed, back to %d\n",
>> > +                     role, current_role);
>> > +             ret = cdns3_role_start(cdns, current_role);
>> > +     }
>> > +
>> > +     return ret;
>> > +}
>> > +
>> > +/**
>> > + * cdns3_role_switch - work queue handler for role switch
>> > + *
>> > + * @work: work queue item structure
>> > + *
>> > + * Handles below events:
>> > + * - Role switch for dual-role devices
>> > + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>> > + */
>> > +static void cdns3_role_switch(struct work_struct *work)
>> > +{
>> > +     enum cdns3_roles role = CDNS3_ROLE_END;
>> > +     struct cdns3 *cdns;
>> > +     bool device, host;
>> > +
>> > +     cdns = container_of(work, struct cdns3, role_switch_wq);
>> > +
>> > +     //TODO: implements this functions.
>> > +     //host = cdns3_is_host(cdns);
>> > +     //device = cdns3_is_device(cdns);
>> > +     host = 1;
>> > +     device = 0;
>> > +
>> > +     if (host)
>> > +             role = CDNS3_ROLE_HOST;
>> > +     else if (device)
>> > +             role = CDNS3_ROLE_GADGET;
>> > +
>> > +     if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>> > +         cdns->role == role)
>> > +             return;
>> > +
>>
>> I think all the below code can be moved to cdns3_do_role_switch().
>>
>> > +     pm_runtime_get_sync(cdns->dev);
>> > +     cdns3_role_stop(cdns);
>> > +
>> > +     if (host) {
>> > +             if (cdns->roles[CDNS3_ROLE_HOST])
>> > +                     cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>> > +             pm_runtime_put_sync(cdns->dev);
>> > +             return;
>> > +     }
>> > +
>> > +     if (device)
>> > +             cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>> > +     else
>> > +             cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>> > +
>> > +     pm_runtime_put_sync(cdns->dev);
>> > +}
>> > +
>> > +/**
>> > + * cdns3_probe - probe for cdns3 core device
>> > + * @pdev: Pointer to cdns3 core platform device
>> > + *
>> > + * Returns 0 on success otherwise negative errno
>> > + */
>> > +static int cdns3_probe(struct platform_device *pdev)
>> > +{
>> > +     struct device *dev = &pdev->dev;
>> > +     struct resource *res;
>> > +     struct cdns3 *cdns;
>> > +     void __iomem *regs;
>> > +     int ret;
>> > +
>> > +     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>> > +     if (!cdns)
>> > +             return -ENOMEM;
>> > +
>> > +     cdns->dev = dev;
>> > +
>> > +     platform_set_drvdata(pdev, cdns);
>> > +
>> > +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> > +     if (!res) {
>> > +             dev_err(dev, "missing IRQ\n");
>> > +             return -ENODEV;
>> > +     }
>> > +     cdns->irq = res->start;
>> > +
>> > +     /*
>> > +      * Request memory region
>> > +      * region-0: xHCI
>> > +      * region-1: Peripheral
>> > +      * region-2: OTG registers
>> > +      */
>>
>> The memory region order is different from the dt-binding.
>> There it is OTG, host(xhci), device (peripheral).
>>
>> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > +     regs = devm_ioremap_resource(dev, res);
>> > +
>> > +     if (IS_ERR(regs))
>> > +             return PTR_ERR(regs);
>> > +     cdns->xhci_regs = regs;
>> > +     cdns->xhci_res = res;
>> > +
>> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> > +     regs = devm_ioremap_resource(dev, res);
>> > +     if (IS_ERR(regs))
>> > +             return PTR_ERR(regs);
>> > +     cdns->dev_regs  = regs;
>> > +
>> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> > +     regs = devm_ioremap_resource(dev, res);
>> > +     if (IS_ERR(regs))
>> > +             return PTR_ERR(regs);
>> > +     cdns->otg_regs = regs;
>> > +
>> > +     mutex_init(&cdns->mutex);
>> > +
>> > +     cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>>
>> "cdns3,usbphy" is not documented in dt-binding.
>>
>> > +     if (IS_ERR(cdns->phy)) {
>> > +             dev_info(dev, "no generic phy found\n");
>> > +             cdns->phy = NULL;
>> > +             /*
>> > +              * fall through here!
>> > +              * if no generic phy found, phy init
>> > +              * should be done under boot!
>> > +              */
>>
>> No you shouldn't fall through always if it is an error condition.
>> Something like this should work better.
>>
>>         if (IS_ERR(cnds->phy)) {
>>                 ret = PTR_ERR(cdns->phy);
>>                 if (ret == -ENOSYS || ret == -ENODEV) {
>>                         cdns->phy = NULL;
>>                 } else if (ret == -EPROBE_DEFER) {
>>                         return ret;
>>                 } else {
>>                         dev_err(dev, "no phy found\n");
>>                         goto err0;
>>                 }
>>         }
>>
>> So if PHY was provided in DT, and PHY support/drivers is present
>> and error condition means something is wrong and we have to error out.
>>
>> > +     } else {
>> > +             phy_init(cdns->phy);
>> > +     }
>>
>> You can do phy_init() outside the else.
>>
>> > +
>> > +     ret = cdns3_core_init_role(cdns);
>> > +     if (ret)
>> > +             goto err1;
>> > +
>> > +     INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>> > +     if (ret)
>> > +             goto err2;
>> > +
>> > +     if (ret)
>> > +             goto err2;
>> > +
>> > +     cdns->role = cdns3_get_role(cdns);
>>
>> I think this should move to cdns3_core_init_role().
>>
>
>I agree.
>
>> > +
>> > +     ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>> > +                            dev_name(dev), cdns);
>> > +
>> > +     if (ret)
>> > +             goto err2;
>>
>> How about moving request_irq to before cdsn3_core_init_role()?
>>
>> Then you can move cdns3_role_start() as well to core_init_role().
>>
>
>Usually, we request irq after hardware initialization has finished, if not,
>there may unexpected interrupt.

>
>Peter
>
>> > +
>> > +     ret = cdns3_role_start(cdns, cdns->role);
>> > +     if (ret) {
>> > +             dev_err(dev, "can't start %s role\n",
>> > +                     cdns3_get_current_role_driver(cdns)->name);
>> > +             goto err2;
>> > +     }
>> > +
>> > +     device_set_wakeup_capable(dev, true);
>> > +     pm_runtime_set_active(dev);
>> > +     pm_runtime_enable(dev);
>> > +
>> > +     /*
>> > +      * The controller needs less time between bus and controller suspend,
>> > +      * and we also needs a small delay to avoid frequently entering low
>> > +      * power mode.
>> > +      */
>> > +     pm_runtime_set_autosuspend_delay(dev, 20);
>> > +     pm_runtime_mark_last_busy(dev);
>> > +     pm_runtime_use_autosuspend(dev);
>> > +     dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
>> > +
>> > +     return 0;
>> > +
>> > +err2:
>> > +     cdns3_remove_roles(cdns);
>> > +err1:
>>
>> phy_exit() ?
>>
>> > +     return ret;
>> > +}
>> > +
>> > +/**
>> > + * cdns3_remove - unbind drd driver and clean up
>> > + * @pdev: Pointer to Linux platform device
>> > + *
>> > + * Returns 0 on success otherwise negative errno
>> > + */
>> > +static int cdns3_remove(struct platform_device *pdev)
>> > +{
>> > +     struct cdns3 *cdns = platform_get_drvdata(pdev);
>> > +
>> > +     pm_runtime_get_sync(&pdev->dev);
>> > +     pm_runtime_disable(&pdev->dev);
>> > +     pm_runtime_put_noidle(&pdev->dev);
>> > +     cdns3_remove_roles(cdns);
>>
>> phy_exit() ?
>>
>> > +
>> > +     return 0;
>> > +}
>> > +
>> > +#ifdef CONFIG_OF
>> > +static const struct of_device_id of_cdns3_match[] = {
>> > +     { .compatible = "cdns,usb3" },
>> > +     { },
>> > +};
>> > +MODULE_DEVICE_TABLE(of, of_cdns3_match);
>> > +#endif
>> > +
>> > +#ifdef CONFIG_PM
>> > +
>> > +#ifdef CONFIG_PM_SLEEP
>> > +static int cdns3_suspend(struct device *dev)
>> > +{
>> > +     //TODO: implements this function
>> > +     return 0;
>> > +}
>> > +
>> > +static int cdns3_resume(struct device *dev)
>> > +{
>> > +     //TODO: implements this function
>> > +     return 0;
>> > +}
>> > +#endif /* CONFIG_PM_SLEEP */
>> > +static int cdns3_runtime_suspend(struct device *dev)
>> > +{    //TODO: implements this function
>> > +     return 0;
>> > +}
>> > +
>> > +static int cdns3_runtime_resume(struct device *dev)
>> > +{
>> > +     //TODO: implements this function
>> > +     return 0;
>> > +}
>> > +#endif /* CONFIG_PM */
>> > +
>> > +static const struct dev_pm_ops cdns3_pm_ops = {
>> > +     SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
>> > +     SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
>> > +};
>> > +
>> > +static struct platform_driver cdns3_driver = {
>> > +     .probe          = cdns3_probe,
>> > +     .remove         = cdns3_remove,
>> > +     .driver         = {
>> > +             .name   = "cdns-usb3",
>> > +             .of_match_table = of_match_ptr(of_cdns3_match),
>> > +             .pm     = &cdns3_pm_ops,
>> > +     },
>> > +};
>> > +
>> > +static int __init cdns3_driver_platform_register(void)
>> > +{
>> > +     return platform_driver_register(&cdns3_driver);
>> > +}
>> > +module_init(cdns3_driver_platform_register);
>> > +
>> > +static void __exit cdns3_driver_platform_unregister(void)
>> > +{
>> > +     platform_driver_unregister(&cdns3_driver);
>> > +}
>> > +module_exit(cdns3_driver_platform_unregister);
>> > +
>> > +MODULE_ALIAS("platform:cdns3");
>> > +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
>> > +MODULE_LICENSE("GPL v2");
>> > +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> > diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> > new file mode 100644
>> > index 000000000000..7c8204fe4d3d
>> > --- /dev/null
>> > +++ b/drivers/usb/cdns3/core.h
>> > @@ -0,0 +1,100 @@
>> > +/* SPDX-License-Identifier: GPL-2.0 */
>> > +/*
>> > + * Cadence USBSS DRD Driver.
>> > + *
>> > + * Copyright (C) 2017 NXP
>> > + * Copyright (C) 2018 Cadence.
>> > + *
>> > + * Authors: Peter Chen <[e-mail]>
>> > + *          Pawel Laszczak <[e-mail]>
>> > + */
>> > +#include <linux/usb/otg.h>
>> > +
>> > +#ifndef __LINUX_CDNS3_CORE_H
>> > +#define __LINUX_CDNS3_CORE_H
>> > +
>> > +struct cdns3;
>> > +enum cdns3_roles {
>> > +     CDNS3_ROLE_HOST = 0,
>> > +     CDNS3_ROLE_GADGET,
>> > +     CDNS3_ROLE_END,
>> > +};
>> > +
>> > +/**
>> > + * struct cdns3_role_driver - host/gadget role driver
>> > + * @start: start this role
>> > + * @stop: stop this role
>> > + * @suspend: suspend callback for this role
>> > + * @resume: resume callback for this role
>> > + * @irq: irq handler for this role
>> > + * @name: role name string (host/gadget)
>> > + */
>> > +struct cdns3_role_driver {
>> > +     int (*start)(struct cdns3 *cdns);
>> > +     void (*stop)(struct cdns3 *cdns);
>> > +     int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
>> > +     int (*resume)(struct cdns3 *cdns, bool hibernated);
>> > +     irqreturn_t (*irq)(struct cdns3 *cdns);
>> > +     const char *name;
>> > +};
>> > +
>> > +#define CDNS3_NUM_OF_CLKS    5
>> > +/**
>> > + * struct cdns3 - Representation of Cadence USB3 DRD controller.
>> > + * @dev: pointer to Cadence device struct
>> > + * @xhci_regs: pointer to base of xhci registers
>> > + * @xhci_res: the resource for xhci
>> > + * @dev_regs: pointer to base of dev registers
>> > + * @otg_regs: pointer to base of otg registers
>> > + * @irq: irq number for controller
>> > + * @roles: array of supported roles for this controller
>> > + * @role: current role
>> > + * @host_dev: the child host device pointer for cdns3 core
>> > + * @gadget_dev: the child gadget device pointer for cdns3 core
>> > + * @usb: phy for this controller
>> > + * @role_switch_wq: work queue item for role switch
>> > + * @in_lpm: the controller in low power mode
>> > + * @wakeup_int: the wakeup interrupt
>> > + * @mutex: the mutex for concurrent code at driver
>> > + * @dr_mode: supported mode of operation it can be only Host, only Device
>> > + *           or OTG mode that allow to switch between Device and Host mode.
>> > + *           This field based on hardware configuration and cant't be changed.
>>
>> But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.
>>
>> > + * @current_dr_role: current mode of operation when in dual-role mode
>> > + * @desired_dr_role: desired mode of operation when in dual-role mode.
>> > + *           This value can be changed during runtime.
>> > + *           Available options depends on  dr_mode:
>> > + *           dr_mode                 |  desired_dr_role and current_dr_role
>> > + *           ----------------------------------------------------------------
>> > + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
>> > + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
>> > + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
>> > + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
>> > + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
>>
>> Do you need to update the right hand side to reflect ROLEs instead of MODE?
>>
>> > + *
>> > + *           Desired_dr_role can be changed by means of debugfs.
>> > + * @root: debugfs root folder pointer
>> > + */
>> > +struct cdns3 {
>> > +     struct device                   *dev;
>> > +     void __iomem                    *xhci_regs;
>> > +     struct resource                 *xhci_res;
>> > +     struct cdns3_usb_regs __iomem   *dev_regs;
>> > +     struct cdns3_otg_regs           *otg_regs;
>> > +     int irq;
>> > +     struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
>> > +     enum cdns3_roles                role;
>> > +     struct device                   *host_dev;
>> > +     struct device                   *gadget_dev;
>> > +     struct phy                      *phy;
>> > +     struct work_struct              role_switch_wq;
>> > +     int                             in_lpm:1;
>> > +     int                             wakeup_int:1;
>> > +     /* mutext used in workqueue*/
>> > +     struct mutex                    mutex;
>> > +     enum usb_dr_mode                dr_mode;
>> > +     enum usb_dr_mode                current_dr_mode;
>> > +     enum usb_dr_mode                desired_dr_mode;
>> > +     struct dentry                   *root;
>> > +};
>> > +
>> > +#endif /* __LINUX_CDNS3_CORE_H */
>> >
>>
>> cheers,
>> -roger
>> --
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Pawel Laszczak Dec. 5, 2018, 7:42 p.m. UTC | #14

+ Tomek Klimek 

>> > +
>> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> > +{
>> > +     enum cdns3_roles role = cdns->role;
>> > +
>> > +     if (role == CDNS3_ROLE_END)
>>
>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>
>> > +             return;
>> > +
>> > +     mutex_lock(&cdns->mutex);
>> > +     cdns->roles[role]->stop(cdns);
>> > +     cdns->role = CDNS3_ROLE_END;
>>
>> Why change the role here? You are just stopping the role not changing it.
>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> if required without error.
>>
>
>The current version of this IP has some issues to detect vbus status correctly,
>we have to force vbus status accordingly, so we need a status to indicate
>vbus disconnection, and add some code to let controller know vbus
>removal, in that case, the controller's state machine can be correct.
>So, we increase one role 'CDNS3_ROLE_END' to for this purpose.

Hi, Tomek do you have any comment for this.
We have in RTL whole OTG machine and we can read all states. 

From otg specification we have in otg_state such bits:
5:3	host_otg_state	"Current state of the OTG Host FSM.
3'b000 : H_IDLE
3'b001 : H_VBUS_ON
3'b010 : H_VBUS_FAILED
3'b011 : H_OTG_HOST_MODE
3'b100 : H_HOST_MODE
3'b101 : H_SWITCH_TO_DEVICE
3'b110 : H_A_SUSPEND
3'b111 : H_WAIT_VBUS_FALL"	RO	0x0
2:0	dev_otg_state	"Current state of the OTG Device FSM.
3'b000 : DEV_IDLE
3'b001 : DEV_MODE
3'b010 : DEV_SRP
3'b011 : DEV_WAIT_VBUS_FALL
3'b100 : DEV_SWITCH_TO_HOST
3'b101 : DEV_WAIT_FOR_CONN"

>
>CDNS3_ROLE_GADGET: gadget mode and VBUS on
>CDNS3_ROLE_HOST: host mode and VBUS on
>CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
>
>So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
>and need to set role as CDNS3_ROLE_END at ->stop for further handling at
>role switch routine.
>

>> > +     mutex_unlock(&cdns->mutex);
>> > +}
>> > +
>> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> > +{
>> > +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> > +             //TODO: implements selecting device/host mode
>> > +             return CDNS3_ROLE_HOST;
>> > +     }
>> > +     return cdns->roles[CDNS3_ROLE_HOST]
>> > +             ? CDNS3_ROLE_HOST
>> > +             : CDNS3_ROLE_GADGET;

Cheers
Pawel,

Pawel Laszczak Dec. 6, 2018, 7 a.m. UTC | #15

>
>> Hi Roger
>>
>> >On 18/11/18 12:09, Pawel Laszczak wrote:
>> >> Patch adds core.c and core.h file that implements initialization
>> >> of platform driver and adds function responsible for selecting,
>> >> switching and running appropriate Device/Host mode.
>> >>
>> >> Signed-off-by: Pawel Laszczak <[e-mail]>
>> >> ---
>> >>  drivers/usb/cdns3/Makefile |   2 +
>> >>  drivers/usb/cdns3/core.c   | 413 +++++++++++++++++++++++++++++++++++++
>> >>  drivers/usb/cdns3/core.h   | 100 +++++++++
>> >>  3 files changed, 515 insertions(+)
>> >>  create mode 100644 drivers/usb/cdns3/core.c
>> >>  create mode 100644 drivers/usb/cdns3/core.h
>> >>
>> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> >> index dcdd62003c6a..02d25b23c5d3 100644
>> >> --- a/drivers/usb/cdns3/Makefile
>> >> +++ b/drivers/usb/cdns3/Makefile
>> >> @@ -1,3 +1,5 @@
>> >> +obj-$(CONFIG_USB_CDNS3)                     += cdns3.o
>> >>  obj-$(CONFIG_USB_CDNS3_PCI_WRAP)    += cdns3-pci.o
>> >>
>> >> +cdns3-y                                     := core.o
>> >>  cdns3-pci-y                         := cdns3-pci-wrap.o
>> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> >> new file mode 100644
>> >> index 000000000000..f9055d4da67f
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.c
>> >> @@ -0,0 +1,413 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >> + * Copyright (C) 2018 Cadence.
>> >> + *
>> >> + * Author: Peter Chen <[e-mail]>
>> >> + *         Pawel Laszczak <[e-mail]>
>> >> + */
>> >> +
>> >> +#include <linux/module.h>
>> >> +#include <linux/kernel.h>
>> >> +#include <linux/platform_device.h>
>> >> +#include <linux/interrupt.h>
>> >> +#include <linux/io.h>
>> >> +#include <linux/pm_runtime.h>
>> >> +
>> >> +#include "gadget.h"
>> >> +#include "core.h"
>> >> +
>> >> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>> >> +{
>> >> +    WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>> >> +    return cdns->roles[cdns->role];
>> >> +}
>> >> +
>> >> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>> >> +{
>> >> +    int ret;
>> >> +
>> >> +    if (role >= CDNS3_ROLE_END)
>> >
>> >WARN_ON()?
>> I agree.
>> >
>> >> +            return 0;
>> >> +
>> >> +    if (!cdns->roles[role])
>> >> +            return -ENXIO;
>> >> +
>> >> +    mutex_lock(&cdns->mutex);
>> >> +    cdns->role = role;
>> >> +    ret = cdns->roles[role]->start(cdns);
>> >> +    mutex_unlock(&cdns->mutex);
>> >> +    return ret;
>> >> +}
>> >> +
>> >> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>> >> +{
>> >> +    enum cdns3_roles role = cdns->role;
>> >> +
>> >> +    if (role == CDNS3_ROLE_END)
>> >
>> >WARN_ON(role >= CNDS3_ROLE_END) ?
>> I agree
>> >
>> >> +            return;
>> >> +
>> >> +    mutex_lock(&cdns->mutex);
>> >> +    cdns->roles[role]->stop(cdns);
>> >> +    cdns->role = CDNS3_ROLE_END;
>> >
>> >Why change the role here? You are just stopping the role not changing it.
>> >I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> >if required without error.
>>
>> This line is unnecessary.
>>
>> >
>> >> +    mutex_unlock(&cdns->mutex);
>> >> +}
>> >> +
>> >> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>> >> +{
>> >> +    if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>> >> +            //TODO: implements selecting device/host mode
>> >> +            return CDNS3_ROLE_HOST;
>> >> +    }
>> >> +    return cdns->roles[CDNS3_ROLE_HOST]
>> >> +            ? CDNS3_ROLE_HOST
>> >> +            : CDNS3_ROLE_GADGET;
>> >
>> >Why not just
>> >       return cdns->role;
>> >
>> >I'm wondering if we really need this function
>>
>> TODO will look likie:
>>                 if (cdns3_is_host(cdns))
>>                         return CDNS3_ROLE_HOST;
>>                 if (cdns3_is_device(cdns))
>>                         return CDNS3_ROLE_GADGET;
>>
>> Function selects initial role. Before invoking it the role is unknown.
>> I think that function name should be  changed because current name can be misleading.
>>
>> I will change it to cdns3_get_initial_role.
>> .
>> >> +}
>> >
>> >> +
>> >> +/**
>> >> + * cdns3_core_init_role - initialize role of operation
>> >> + * @cdns: Pointer to cdns3 structure
>> >> + *
>> >> + * Returns 0 on success otherwise negative errno
>> >> + */
>> >> +static int cdns3_core_init_role(struct cdns3 *cdns)
>> >> +{
>> >> +    struct device *dev = cdns->dev;
>> >> +    enum usb_dr_mode dr_mode;
>> >> +
>> >> +    dr_mode = usb_get_dr_mode(dev);
>> >> +    cdns->role = CDNS3_ROLE_END;
>> >> +
>> >> +    /*
>> >> +     * If driver can't read mode by means of usb_get_dr_mdoe function then
>> >> +     * chooses mode according with Kernel configuration. This setting
>> >> +     * can be restricted later depending on strap pin configuration.
>> >> +     */
>> >> +    if (dr_mode == USB_DR_MODE_UNKNOWN) {
>> >> +            if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>> >> +                IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> >> +                    dr_mode = USB_DR_MODE_OTG;
>> >> +            else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>> >> +                    dr_mode = USB_DR_MODE_HOST;
>> >> +            else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>> >> +                    dr_mode = USB_DR_MODE_PERIPHERAL;
>> >> +    }
>> >> +
>> >> +    if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>> >> +            //TODO: implements host initialization
>> >
>> >               /* TODO: Add host role */ ?
>> >
>> >> +    }
>> >> +
>> >> +    if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>> >> +            //TODO: implements device initialization
>> >
>> >               /* TODO: Add device role */ ?
>> >
>> >> +    }
>> >> +
>> >> +    if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>> >> +            dev_err(dev, "no supported roles\n");
>> >> +            return -ENODEV;
>> >> +    }
>> >> +
>> >> +    cdns->dr_mode = dr_mode;
>> >> +    return 0;
>> >> +}
>> >> +
>> >> +/**
>> >> + * cdns3_irq - interrupt handler for cdns3 core device
>> >> + *
>> >> + * @irq: irq number for cdns3 core device
>> >> + * @data: structure of cdns3
>> >> + *
>> >> + * Returns IRQ_HANDLED or IRQ_NONE
>> >> + */
>> >> +static irqreturn_t cdns3_irq(int irq, void *data)
>> >> +{
>> >> +    struct cdns3 *cdns = data;
>> >> +    irqreturn_t ret = IRQ_NONE;
>> >> +
>> >> +    /* Handle device/host interrupt */
>> >> +    if (cdns->role != CDNS3_ROLE_END)
>> >
>> >Is it because of this that you need to set role to END at role_stop?
>> >I think it is better to add a state variable to struct cdns3_role_driver, so we can
>> >check if it is active or stopped.
>> >
>> >e.g.
>> >       if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>>
>> Ok, I will do it in this way.
>> >> +            ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>> >> +
>> >> +    return ret;
>> >> +}
>> >> +
>> >> +static void cdns3_remove_roles(struct cdns3 *cdns)
>> >
>> >Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>>
>> Sounds better.
>> I also change cdns3_host_remove to cdns3_host_exit and
>> cdns3_gadget_remove to cdns3_gadget_exit.
>> >
>> >> +{
>> >> +    //TODO: implements this function
>> >> +}
>> >
>> >> +
>> >> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>> >> +{
>> >> +    enum cdns3_roles current_role;
>> >> +    int ret = 0;
>> >> +
>> >> +    current_role = cdns->role;
>> >> +
>> >> +    if (role == CDNS3_ROLE_END)
>> >> +            return 0;
>> >
>> >role == END looks like error state. and it should never happen.
>> >WARN here?
>>
>> Ok, will be changed.
>> >
>
>Please consider my comments which replied to Roger just now.

I  try to think also about you platform. Personally I think that someone from 
us should have access to both platforms in order to create one common driver.   
I use drd.c  file for detecting  ID pin. As I know  you use external connection class for
Detecting host/device role. 
My goals is not to create the common driver for our different platforms  because 
I don’t have access for your testing platform. My goals is to create driver that you 
could easily  adapt to your platform, so all your comment are very valuable for me. 
 
>
>> >> +
>> >> +    dev_dbg(cdns->dev, "Switching role");
>> >> +
>> >
>> >Don't you have to stop the previous role before starting the new role?
>> >
>> >> +    ret = cdns3_role_start(cdns, role);
>> >> +    if (ret) {
>> >> +            /* Back to current role */
>> >> +            dev_err(cdns->dev, "set %d has failed, back to %d\n",
>> >> +                    role, current_role);
>> >> +            ret = cdns3_role_start(cdns, current_role);
>> >> +    }
>> >> +
>> >> +    return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * cdns3_role_switch - work queue handler for role switch
>> >> + *
>> >> + * @work: work queue item structure
>> >> + *
>> >> + * Handles below events:
>> >> + * - Role switch for dual-role devices
>> >> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>> >> + */
>> >> +static void cdns3_role_switch(struct work_struct *work)
>> >> +{
>> >> +    enum cdns3_roles role = CDNS3_ROLE_END;
>> >> +    struct cdns3 *cdns;
>> >> +    bool device, host;
>> >> +
>> >> +    cdns = container_of(work, struct cdns3, role_switch_wq);
>> >> +
>> >> +    //TODO: implements this functions.
>> >> +    //host = cdns3_is_host(cdns);
>> >> +    //device = cdns3_is_device(cdns);
>> >> +    host = 1;
>> >> +    device = 0;
>> >> +
>> >> +    if (host)
>> >> +            role = CDNS3_ROLE_HOST;
>> >> +    else if (device)
>> >> +            role = CDNS3_ROLE_GADGET;
>> >> +
>> >> +    if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>> >> +        cdns->role == role)
>> >> +            return;
>> >> +
>> >
>> >I think all the below code can be moved to cdns3_do_role_switch().
>>
>> Yes, I agree with you. cdns3_role_stop  should be in cdns3_do_role_switch.
>>
>> >> +    pm_runtime_get_sync(cdns->dev);
>> >> +    cdns3_role_stop(cdns);
>> >> +
>> >> +    if (host) {
>> >> +            if (cdns->roles[CDNS3_ROLE_HOST])
>> >> +                    cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>> >> +            pm_runtime_put_sync(cdns->dev);
>> >> +            return;
>> >> +    }
>> >> +
>> >> +    if (device)
>> >> +            cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>> >> +    else
>> >> +            cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>> >> +
>> >> +    pm_runtime_put_sync(cdns->dev);
>> >> +}
>> >> +
>> >> +/**
>> >> + * cdns3_probe - probe for cdns3 core device
>> >> + * @pdev: Pointer to cdns3 core platform device
>> >> + *
>> >> + * Returns 0 on success otherwise negative errno
>> >> + */
>> >> +static int cdns3_probe(struct platform_device *pdev)
>> >> +{
>> >> +    struct device *dev = &pdev->dev;
>> >> +    struct resource *res;
>> >> +    struct cdns3 *cdns;
>> >> +    void __iomem *regs;
>> >> +    int ret;
>> >> +
>> >> +    cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>> >> +    if (!cdns)
>> >> +            return -ENOMEM;
>> >> +
>> >> +    cdns->dev = dev;
>> >> +
>> >> +    platform_set_drvdata(pdev, cdns);
>> >> +
>> >> +    res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> >> +    if (!res) {
>> >> +            dev_err(dev, "missing IRQ\n");
>> >> +            return -ENODEV;
>> >> +    }
>> >> +    cdns->irq = res->start;
>> >> +
>> >> +    /*
>> >> +     * Request memory region
>> >> +     * region-0: xHCI
>> >> +     * region-1: Peripheral
>> >> +     * region-2: OTG registers
>> >> +     */
>> >
>> >The memory region order is different from the dt-binding.
>> >There it is OTG, host(xhci), device (peripheral).
>>
>> I corrected dt-binding.
>> >
>> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> +    regs = devm_ioremap_resource(dev, res);
>> >> +
>> >> +    if (IS_ERR(regs))
>> >> +            return PTR_ERR(regs);
>> >> +    cdns->xhci_regs = regs;
>> >> +    cdns->xhci_res = res;
>> >> +
>> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> >> +    regs = devm_ioremap_resource(dev, res);
>> >> +    if (IS_ERR(regs))
>> >> +            return PTR_ERR(regs);
>> >> +    cdns->dev_regs  = regs;
>> >> +
>> >> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> >> +    regs = devm_ioremap_resource(dev, res);
>> >> +    if (IS_ERR(regs))
>> >> +            return PTR_ERR(regs);
>> >> +    cdns->otg_regs = regs;
>> >> +
>> >> +    mutex_init(&cdns->mutex);
>> >> +
>> >> +    cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>> >
>> >"cdns3,usbphy" is not documented in dt-binding.
>>
>> I assume that I should add  to dt-binding (cdns3-usb.txt) something like:
>>  - phys: reference to the USB PHY
>>  - phy-names: name of the USB PHY, should be " cdns3,usbphy "
>>
>> >
>> >> +    if (IS_ERR(cdns->phy)) {
>> >> +            dev_info(dev, "no generic phy found\n");
>> >> +            cdns->phy = NULL;
>> >> +            /*
>> >> +             * fall through here!
>> >> +             * if no generic phy found, phy init
>> >> +             * should be done under boot!
>> >> +             */
>> >
>> >No you shouldn't fall through always if it is an error condition.
>> >Something like this should work better.
>> >
>> >        if (IS_ERR(cnds->phy)) {
>> >                ret = PTR_ERR(cdns->phy);
>> >                if (ret == -ENOSYS || ret == -ENODEV) {
>> >                        cdns->phy = NULL;
>> >                } else if (ret == -EPROBE_DEFER) {
>> >                        return ret;
>> >                } else {
>> >                        dev_err(dev, "no phy found\n");
>> >                        goto err0;
>> >                }
>> >        }
>> >
>> >So if PHY was provided in DT, and PHY support/drivers is present
>> >and error condition means something is wrong and we have to error out.
>> >
>> >> +    } else {
>> >> +            phy_init(cdns->phy);
>> >> +    }
>> >
>> >You can do phy_init() outside the else.
>> >
>> Thank you for explanation. I will correct this.
>> >> +
>> >> +    ret = cdns3_core_init_role(cdns);
>> >> +    if (ret)
>> >> +            goto err1;
>> >> +
>> >> +    INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>> >> +    if (ret)
>> >> +            goto err2;
>> >> +
>> >> +    if (ret)
>> >> +            goto err2;
>> >> +
>> >> +    cdns->role = cdns3_get_role(cdns);
>> >
>> >I think this should move to cd I'll have a some though on ns3_core_init_role().
>>
>> Ok, I will do it.
>> >
>> >> +
>> >> +    ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>> >> +                           dev_name(dev), cdns);
>> >> +
>> >> +    if (ret)
>> >> +            goto err2;
>> >
>> >How about moving request_irq to before cdsn3_core_init_role()?
>> >
>> >Then you can move cdns3_role_start() as well to core_init_role().
>> I'll give it  some though on it, but probably I will probably have to change little other function..
>> So the new order should look like this:
>>
>> cdns3_drd_init
>> devm_request_irq
>> cdns3_core_init_role, cdns3_get_role, cdns3_role_start
>>
>> >
>> >> +
>> >> +    ret = cdns3_role_start(cdns, cdns->role);
>> >> +    if (ret) {
>> >> +            dev_err(dev, "can't start %s role\n",
>> >> +                    cdns3_get_current_role_driver(cdns)->name);
>> >> +            goto err2;
>> >> +    }
>> >> +
>> >> +    device_set_wakeup_capable(dev, true);
>> >> +    pm_runtime_set_active(dev);
>> >> +    pm_runtime_enable(dev);
>> >> +
>> >> +    /*
>> >> +     * The controller needs less time between bus and controller suspend,
>> >> +     * and we also needs a small delay to avoid frequently entering low
>> >> +     * power mode.
>> >> +     */
>> >> +    pm_runtime_set_autosuspend_delay(dev, 20);
>> >> +    pm_runtime_mark_last_busy(dev);
>> >> +    pm_runtime_use_autosuspend(dev);
>> >> +    dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
>> >> +
>> >> +    return 0;
>> >> +
>> >> +err2:
>> >> +    cdns3_remove_roles(cdns);
>> >> +err1:
>> >
>> >phy_exit() ?
>> I will add.
>> >
>> >> +    return ret;
>> >> +}
>> >> +
>> >> +/**
>> >> + * cdns3_remove - unbind drd driver and clean up
>> >> + * @pdev: Pointer to Linux platform device
>> >> + *
>> >> + * Returns 0 on success otherwise negative errno
>> >> + */
>> >> +static int cdns3_remove(struct platform_device *pdev)
>> >> +{
>> >> +    struct cdns3 *cdns = platform_get_drvdata(pdev);
>> >> +
>> >> +    pm_runtime_get_sync(&pdev->dev);
>> >> +    pm_runtime_disable(&pdev->dev);
>> >> +    pm_runtime_put_noidle(&pdev->dev);
>> >> +    cdns3_remove_roles(cdns);
>> >
>> >phy_exit() ?
>> I will add.
>> >
>> >> +
>> >> +    return 0;
>> >> +}
>> >> +
>> >> +#ifdef CONFIG_OF
>> >> +static const struct of_device_id of_cdns3_match[] = {
>> >> +    { .compatible = "cdns,usb3" },
>> >> +    { },
>> >> +};
>> >> +MODULE_DEVICE_TABLE(of, of_cdns3_match);
>> >> +#endif
>> >> +
>> >> +#ifdef CONFIG_PM
>> >> +
>> >> +#ifdef CONFIG_PM_SLEEP
>> >> +static int cdns3_suspend(struct device *dev)
>> >> +{
>> >> +    //TODO: implements this function
>> >> +    return 0;
>> >> +}
>> >> +
>> >> +static int cdns3_resume(struct device *dev)
>> >> +{
>> >> +    //TODO: implements this function
>> >> +    return 0;
>> >> +}
>> >> +#endif /* CONFIG_PM_SLEEP */
>> >> +static int cdns3_runtime_suspend(struct device *dev)
>> >> +{   //TODO: implements this function
>> >> +    return 0;
>> >> +}
>> >> +
>> >> +static int cdns3_runtime_resume(struct device *dev)
>> >> +{
>> >> +    //TODO: implements this function
>> >> +    return 0;
>> >> +}
>> >> +#endif /* CONFIG_PM */
>> >> +
>> >> +static const struct dev_pm_ops cdns3_pm_ops = {
>> >> +    SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
>> >> +    SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
>> >> +};
>> >> +
>> >> +static struct platform_driver cdns3_driver = {
>> >> +    .probe          = cdns3_probe,
>> >> +    .remove         = cdns3_remove,
>> >> +    .driver         = {
>> >> +            .name   = "cdns-usb3",
>> >> +            .of_match_table = of_match_ptr(of_cdns3_match),
>> >> +            .pm     = &cdns3_pm_ops,
>> >> +    },
>> >> +};
>> >> +
>> >> +static int __init cdns3_driver_platform_register(void)
>> >> +{
>> >> +    return platform_driver_register(&cdns3_driver);
>> >> +}
>> >> +module_init(cdns3_driver_platform_register);
>> >> +
>> >> +static void __exit cdns3_driver_platform_unregister(void)
>> >> +{
>> >> +    platform_driver_unregister(&cdns3_driver);
>> >> +}
>> >> +module_exit(cdns3_driver_platform_unregister);
>> >> +
>> >> +MODULE_ALIAS("platform:cdns3");
>> >> +MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
>> >> +MODULE_LICENSE("GPL v2");
>> >> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> >> new file mode 100644
>> >> index 000000000000..7c8204fe4d3d
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.h
>> >> @@ -0,0 +1,100 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >> + * Copyright (C) 2017 NXP
>> >> + * Copyright (C) 2018 Cadence.
>> >> + *
>> >> + * Authors: Peter Chen <[e-mail]>
>> >> + *          Pawel Laszczak <[e-mail]>
>> >> + */
>> >> +#include <linux/usb/otg.h>
>> >> +
>> >> +#ifndef __LINUX_CDNS3_CORE_H
>> >> +#define __LINUX_CDNS3_CORE_H
>> >> +
>> >> +struct cdns3;
>> >> +enum cdns3_roles {
>> >> +    CDNS3_ROLE_HOST = 0,
>> >> +    CDNS3_ROLE_GADGET,
>> >> +    CDNS3_ROLE_END,
>> >> +};
>> >> +
>> >> +/**
>> >> + * struct cdns3_role_driver - host/gadget role driver
>> >> + * @start: start this role
>> >> + * @stop: stop this role
>> >> + * @suspend: suspend callback for this role
>> >> + * @resume: resume callback for this role
>> >> + * @irq: irq handler for this role
>> >> + * @name: role name string (host/gadget)
>> >> + */
>> >> +struct cdns3_role_driver {
>> >> +    int (*start)(struct cdns3 *cdns);
>> >> +    void (*stop)(struct cdns3 *cdns);
>> >> +    int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
>> >> +    int (*resume)(struct cdns3 *cdns, bool hibernated);
>> >> +    irqreturn_t (*irq)(struct cdns3 *cdns);
>> >> +    const char *name;
>> >> +};
>> >> +
>> >> +#define CDNS3_NUM_OF_CLKS   5
>> >> +/**
>> >> + * struct cdns3 - Representation of Cadence USB3 DRD controller.
>> >> + * @dev: pointer to Cadence device struct
>> >> + * @xhci_regs: pointer to base of xhci registers
>> >> + * @xhci_res: the resource for xhci
>> >> + * @dev_regs: pointer to base of dev registers
>> >> + * @otg_regs: pointer to base of otg registers
>> >> + * @irq: irq number for controller
>> >> + * @roles: array of supported roles for this controller
>> >> + * @role: current role
>> >> + * @host_dev: the child host device pointer for cdns3 core
>> >> + * @gadget_dev: the child gadget device pointer for cdns3 core
>> >> + * @usb: phy for this controller
>> >> + * @role_switch_wq: work queue item for role switch
>> >> + * @in_lpm: the controller in low power mode
>> >> + * @wakeup_int: the wakeup interrupt
>> >> + * @mutex: the mutex for concurrent code at driver
>> >> + * @dr_mode: supported mode of operation it can be only Host, only Device
>> >> + *           or OTG mode that allow to switch between Device and Host mode.
>> >> + *           This field based on hardware configuration and cant't be changed.
>> >
>> >But dr_mode can be forced in device-tree. So it isn't really only hardware configuration.
>> Right, I added dr_mode to dt-binding , so we have STRAP bits in registers and additionally
>> optional dr_mode in device-tree. Driver should take into account this two options.
>> I will remove this line.
>>
>> >
>> >> + * @current_dr_role: current mode of operation when in dual-role mode
>> >> + * @desired_dr_role: desired mode of operation when in dual-role mode.
>> >> + *           This value can be changed during runtime.
>> >> + *           Available options depends on  dr_mode:
>> >> + *           dr_mode                 |  desired_dr_role and current_dr_role
>> >> + *           ----------------------------------------------------------------
>> >> + *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
>> >> + *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
>> >> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
>> >> + *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
>> >> + *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
>> >
>> >Do you need to update the right hand side to reflect ROLEs instead of MODE?
>>
>> I see that there are incorrect name. There should be mode instead role.
>> In structure below the names are correct.
>> >
>
>Usually, we have two type of roles: init role and current role
>init role is decided by dr_mode from firmware, hardware setting and
>kernel configuration together.
>It is decided at ->probe, if above three settings haveconflict, we
>need show an error.
>current role is decided at run time, and only used at dual-role mode.
>For peripheral-only and host-only application, the current role equals
>to init role.
>

I agree. 

But by conflict I understand that we can't chose anyone option. 

e.g.  
Firmware:   OTG
Hardware :  only device
Kernel :   only host 
We can't choose the option. 

But in case 
Firmware:   OTG
Hardware :  only device
Kernel :   only device 
Then we can set dr_mode to HOST device mode and init role will be Host role.


>Peter
>
>> >> + *
>> >> + *           Desired_dr_role can be changed by means of debugfs.
>> >> + * @root: debugfs root folder pointer
>> >> + */
>> >> +struct cdns3 {
>> >> +    struct device                   *dev;
>> >> +    void __iomem                    *xhci_regs;
>> >> +    struct resource                 *xhci_res;
>> >> +    struct cdns3_usb_regs __iomem   *dev_regs;
>> >> +    struct cdns3_otg_regs           *otg_regs;
>> >> +    int irq;
>> >> +    struct cdns3_role_driver        *roles[CDNS3_ROLE_END];
>> >> +    enum cdns3_roles                role;
>> >> +    struct device                   *host_dev;
>> >> +    struct device                   *gadget_dev;
>> >> +    struct phy                      *phy;
>> >> +    struct work_struct              role_switch_wq;
>> >> +    int                             in_lpm:1;
>> >> +    int                             wakeup_int:1;
>> >> +    /* mutext used in workqueue*/
>> >> +    struct mutex                    mutex;
>> >> +    enum usb_dr_mode                dr_mode;
>> >> +    enum usb_dr_mode                current_dr_mode;
>> >> +    enum usb_dr_mode                desired_dr_mode;
>> >> +    struct dentry                   *root;
>> >> +};
>> >> +
>> >> +#endif /* __LINUX_CDNS3_CORE_H */
>> >>
>> >
>> >cheers,
>> >-roger
>> >--
>> >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> Thank for all your comments,
>> Cheers,
>> Pawel

Pawel Laszczak Dec. 6, 2018, 9:31 a.m. UTC | #16

Hi,

>On 04/12/18 10:50, Peter Chen wrote:
>>>> + * Cadence USBSS DRD Driver.
>>>> + *
>>>> + * Copyright (C) 2018 Cadence.
>>>> + *
>>>> + * Author: Peter Chen <[e-mail]>
>>>> + *         Pawel Laszczak <[e-mail]>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +
>>>> +#include "gadget.h"
>>>> +#include "core.h"
>>>> +
>>>> +static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>>> +{
>>>> +     WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>>>> +     return cdns->roles[cdns->role];
>>>> +}
>>>> +
>>>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     if (role >= CDNS3_ROLE_END)
>>>
>>> WARN_ON()?
>>>
>>>> +             return 0;
>>>> +
>>>> +     if (!cdns->roles[role])
>>>> +             return -ENXIO;
>>>> +
>>>> +     mutex_lock(&cdns->mutex);
>>>> +     cdns->role = role;
>>>> +     ret = cdns->roles[role]->start(cdns);
>>>> +     mutex_unlock(&cdns->mutex);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>>>> +{
>>>> +     enum cdns3_roles role = cdns->role;
>>>> +
>>>> +     if (role == CDNS3_ROLE_END)
>>>
>>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>>
>>>> +             return;
>>>> +
>>>> +     mutex_lock(&cdns->mutex);
>>>> +     cdns->roles[role]->stop(cdns);
>>>> +     cdns->role = CDNS3_ROLE_END;
>>>
>>> Why change the role here? You are just stopping the role not changing it.
>>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>>> if required without error.
>>>
>>
>> The current version of this IP has some issues to detect vbus status correctly,
>> we have to force vbus status accordingly, so we need a status to indicate
>> vbus disconnection, and add some code to let controller know vbus
>> removal, in that case, the controller's state machine can be correct.
>> So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
>>
>> CDNS3_ROLE_GADGET: gadget mode and VBUS on
>> CDNS3_ROLE_HOST: host mode and VBUS on
>> CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
>>
>> So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
>> and need to set role as CDNS3_ROLE_END at ->stop for further handling at
>> role switch routine.
>
>OK. but still this (changing to ROLE_END) must be moved to the role switch routine
>and the explanation you just mentioned must be added there.
>
>>
>>>> +     mutex_unlock(&cdns->mutex);
>>>> +}
>>>> +
>>>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>>> +{
>>>> +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>>>> +             //TODO: implements selecting device/host mode
>>>> +             return CDNS3_ROLE_HOST;
>>>> +     }
>>>> +     return cdns->roles[CDNS3_ROLE_HOST]
>>>> +             ? CDNS3_ROLE_HOST
>>>> +             : CDNS3_ROLE_GADGET;
>>>
>>> Why not just
>>>         return cdns->role;
>>>
>>> I'm wondering if we really need this function.
>>
>> cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
>> If both roles are supported, the role is decided by external
>> conditions, eg, vbus/id
>> or external connector. If only single role is supported, only one role structure
>> is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
>>
>
>How about adding this description in function documentation.

Ok, I will do it. 
>
>>>> +}
>>>
>>>> +
>>>> +/**
>>>> + * cdns3_core_init_role - initialize role of operation
>>>> + * @cdns: Pointer to cdns3 structure
>>>> + *
>>>> + * Returns 0 on success otherwise negative errno
>>>> + */
>>>> +static int cdns3_core_init_role(struct cdns3 *cdns)
>>>> +{
>>>> +     struct device *dev = cdns->dev;
>>>> +     enum usb_dr_mode dr_mode;
>>>> +
>>>> +     dr_mode = usb_get_dr_mode(dev);
>>>> +     cdns->role = CDNS3_ROLE_END;
>>>> +
>>>> +     /*
>>>> +      * If driver can't read mode by means of usb_get_dr_mdoe function then
>>>> +      * chooses mode according with Kernel configuration. This setting
>>>> +      * can be restricted later depending on strap pin configuration.
>>>> +      */
>>>> +     if (dr_mode == USB_DR_MODE_UNKNOWN) {
>>>> +             if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>>>> +                 IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>>> +                     dr_mode = USB_DR_MODE_OTG;
>>>> +             else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>>>> +                     dr_mode = USB_DR_MODE_HOST;
>>>> +             else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>>> +                     dr_mode = USB_DR_MODE_PERIPHERAL;
>>>> +     }
>>>> +
>>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>> +             //TODO: implements host initialization
>>>
>>>                 /* TODO: Add host role */ ?
>>>
>>>> +     }
>>>> +
>>>> +     if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>>>> +             //TODO: implements device initialization
>>>
>>>                 /* TODO: Add device role */ ?
>>>
>>
>> Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
>> cdns->roles[CDNS3_ROLE_GADGET].
>>
>>>> +     }
>>>> +
>>>> +     if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
>>>> +             dev_err(dev, "no supported roles\n");
>>>> +             return -ENODEV;
>>>> +     }
>>>> +
>>>> +     cdns->dr_mode = dr_mode;
>>
>> Pawel, why dr_mode needs to be introduced?
>>
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * cdns3_irq - interrupt handler for cdns3 core device
>>>> + *
>>>> + * @irq: irq number for cdns3 core device
>>>> + * @data: structure of cdns3
>>>> + *
>>>> + * Returns IRQ_HANDLED or IRQ_NONE
>>>> + */
>>>> +static irqreturn_t cdns3_irq(int irq, void *data)
>>>> +{
>>>> +     struct cdns3 *cdns = data;
>>>> +     irqreturn_t ret = IRQ_NONE;
>>>> +
>>>> +     /* Handle device/host interrupt */
>>>> +     if (cdns->role != CDNS3_ROLE_END)
>>>
>>> Is it because of this that you need to set role to END at role_stop?
>>> I think it is better to add a state variable to struct cdns3_role_driver, so we can
>>> check if it is active or stopped.
>>>
>>> e.g.
>>>         if (cdns3_get_current_role_driver(cdns)->state == CDNS3_ROLE_STATE_ACTIVE)
>>>
>>>> +             ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>
>>  CDNS3_ROLE_END is introduced from above comments, we don't
>> need another flag for it.
>> If cdns->role == CDNS3_ROLE_END, it handles VBUS and ID interrupt.
>>
>>>> +static void cdns3_remove_roles(struct cdns3 *cdns)
>>>
>>> Should this be called cdns3_exit_roles() to be opposite of cdns3_init_roles()?
>>>
>>
>> It is planed to called when at ->remove.
>>>> +{
>>>> +     //TODO: implements this function
>>>> +}
>>>
>>>> +
>>>> +static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>>>> +{
>>>> +     enum cdns3_roles current_role;
>>>> +     int ret = 0;
>>>> +
>>>> +     current_role = cdns->role;
>>>> +
>>>> +     if (role == CDNS3_ROLE_END)
>>>> +             return 0;
>>>
>>> role == END looks like error state. and it should never happen.
>>> WARN here?
>>>
>>
>> See my comments above.
>>
>>>> +
>>>> +     dev_dbg(cdns->dev, "Switching role");
>>>> +
>>>
>>> Don't you have to stop the previous role before starting the new role?
>>>
>>
>> Yes, it is needed. Pawel may simply some flows to suit his platform.
>>
>>>> +     ret = cdns3_role_start(cdns, role);
>>>> +     if (ret) {
>>>> +             /* Back to current role */
>>>> +             dev_err(cdns->dev, "set %d has failed, back to %d\n",
>>>> +                     role, current_role);
>>>> +             ret = cdns3_role_start(cdns, current_role);
>>>> +     }
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * cdns3_role_switch - work queue handler for role switch
>>>> + *
>>>> + * @work: work queue item structure
>>>> + *
>>>> + * Handles below events:
>>>> + * - Role switch for dual-role devices
>>>> + * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
>>>> + */
>>>> +static void cdns3_role_switch(struct work_struct *work)
>>>> +{
>>>> +     enum cdns3_roles role = CDNS3_ROLE_END;
>>>> +     struct cdns3 *cdns;
>>>> +     bool device, host;
>>>> +
>>>> +     cdns = container_of(work, struct cdns3, role_switch_wq);
>>>> +
>>>> +     //TODO: implements this functions.
>>>> +     //host = cdns3_is_host(cdns);
>>>> +     //device = cdns3_is_device(cdns);
>>>> +     host = 1;
>>>> +     device = 0;
>>>> +
>>>> +     if (host)
>>>> +             role = CDNS3_ROLE_HOST;
>>>> +     else if (device)
>>>> +             role = CDNS3_ROLE_GADGET;
>>>> +
>>>> +     if (cdns->desired_dr_mode == cdns->current_dr_mode &&
>>>> +         cdns->role == role)
>>>> +             return;
>>>> +
>>>
>>> I think all the below code can be moved to cdns3_do_role_switch().
>>>
>>>> +     pm_runtime_get_sync(cdns->dev);
>>>> +     cdns3_role_stop(cdns);
>>>> +
>>>> +     if (host) {
>>>> +             if (cdns->roles[CDNS3_ROLE_HOST])
>>>> +                     cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
>>>> +             pm_runtime_put_sync(cdns->dev);
>>>> +             return;
>>>> +     }
>>>> +
>>>> +     if (device)
>>>> +             cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
>>>> +     else
>>>> +             cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
>>>> +
>>>> +     pm_runtime_put_sync(cdns->dev);
>>>> +}
>>>> +
>>>> +/**
>>>> + * cdns3_probe - probe for cdns3 core device
>>>> + * @pdev: Pointer to cdns3 core platform device
>>>> + *
>>>> + * Returns 0 on success otherwise negative errno
>>>> + */
>>>> +static int cdns3_probe(struct platform_device *pdev)
>>>> +{
>>>> +     struct device *dev = &pdev->dev;
>>>> +     struct resource *res;
>>>> +     struct cdns3 *cdns;
>>>> +     void __iomem *regs;
>>>> +     int ret;
>>>> +
>>>> +     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
>>>> +     if (!cdns)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     cdns->dev = dev;
>>>> +
>>>> +     platform_set_drvdata(pdev, cdns);
>>>> +
>>>> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>> +     if (!res) {
>>>> +             dev_err(dev, "missing IRQ\n");
>>>> +             return -ENODEV;
>>>> +     }
>>>> +     cdns->irq = res->start;
>>>> +
>>>> +     /*
>>>> +      * Request memory region
>>>> +      * region-0: xHCI
>>>> +      * region-1: Peripheral
>>>> +      * region-2: OTG registers
>>>> +      */
>>>
>>> The memory region order is different from the dt-binding.
>>> There it is OTG, host(xhci), device (peripheral).
>>>
>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +     regs = devm_ioremap_resource(dev, res);
>>>> +
>>>> +     if (IS_ERR(regs))
>>>> +             return PTR_ERR(regs);
>>>> +     cdns->xhci_regs = regs;
>>>> +     cdns->xhci_res = res;
>>>> +
>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +     regs = devm_ioremap_resource(dev, res);
>>>> +     if (IS_ERR(regs))
>>>> +             return PTR_ERR(regs);
>>>> +     cdns->dev_regs  = regs;
>>>> +
>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +     regs = devm_ioremap_resource(dev, res);
>>>> +     if (IS_ERR(regs))
>>>> +             return PTR_ERR(regs);
>>>> +     cdns->otg_regs = regs;
>>>> +
>>>> +     mutex_init(&cdns->mutex);
>>>> +
>>>> +     cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>>>
>>> "cdns3,usbphy" is not documented in dt-binding.
>>>
>>>> +     if (IS_ERR(cdns->phy)) {
>>>> +             dev_info(dev, "no generic phy found\n");
>>>> +             cdns->phy = NULL;
>>>> +             /*
>>>> +              * fall through here!
>>>> +              * if no generic phy found, phy init
>>>> +              * should be done under boot!
>>>> +              */
>>>
>>> No you shouldn't fall through always if it is an error condition.
>>> Something like this should work better.
>>>
>>>         if (IS_ERR(cnds->phy)) {
>>>                 ret = PTR_ERR(cdns->phy);
>>>                 if (ret == -ENOSYS || ret == -ENODEV) {
>>>                         cdns->phy = NULL;
>>>                 } else if (ret == -EPROBE_DEFER) {
>>>                         return ret;
>>>                 } else {
>>>                         dev_err(dev, "no phy found\n");
>>>                         goto err0;
>>>                 }
>>>         }
>>>
>>> So if PHY was provided in DT, and PHY support/drivers is present
>>> and error condition means something is wrong and we have to error out.
>>>
>>>> +     } else {
>>>> +             phy_init(cdns->phy);
>>>> +     }
>>>
>>> You can do phy_init() outside the else.
>>>
>>>> +
>>>> +     ret = cdns3_core_init_role(cdns);
>>>> +     if (ret)
>>>> +             goto err1;
>>>> +
>>>> +     INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
>>>> +     if (ret)
>>>> +             goto err2;
>>>> +
>>>> +     if (ret)
>>>> +             goto err2;
>>>> +
>>>> +     cdns->role = cdns3_get_role(cdns);
>>>
>>> I think this should move to cdns3_core_init_role().
>>>
>>
>> I agree.
>>
>>>> +
>>>> +     ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
>>>> +                            dev_name(dev), cdns);
>>>> +
>>>> +     if (ret)
>>>> +             goto err2;
>>>
>>> How about moving request_irq to before cdsn3_core_init_role()?
>>>
>>> Then you can move cdns3_role_start() as well to core_init_role().
>>>
>>
>> Usually, we request irq after hardware initialization has finished, if not,
>> there may unexpected interrupt.
>
>Doesn't kernel warn if interrupt happens and there is no handler?
>To avoid that I was suggesting to request_irq first.
>
Cheers
Pawel

Pawel Laszczak Dec. 6, 2018, 10:02 a.m. UTC | #17

Hi,

>>> > +
>>> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
>>> > +{
>>> > +     enum cdns3_roles role = cdns->role;
>>> > +
>>> > +     if (role == CDNS3_ROLE_END)
>>>
>>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>>
>>> > +             return;
>>> > +
>>> > +     mutex_lock(&cdns->mutex);
>>> > +     cdns->roles[role]->stop(cdns);
>>> > +     cdns->role = CDNS3_ROLE_END;
>>>
>>> Why change the role here? You are just stopping the role not changing it.
>>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>>> if required without error.
>>>
>>
>>The current version of this IP has some issues to detect vbus status correctly,
>>we have to force vbus status accordingly, so we need a status to indicate
>>vbus disconnection, and add some code to let controller know vbus
>>removal, in that case, the controller's state machine can be correct.
>>So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
>
>Hi, Tomek do you have any comment for this.
>We have in RTL whole OTG machine and we can read all states.

It's not the IP issue, but with PHY. I told with Tomek and he confirmed this issue.
In my testing platform I use different phy version and I don't have such issue. 

CDNS3_ROLE_END stay in driver for compatibility with Peter PHY version. 

>From otg specification we have in otg_state such bits:
>5:3	host_otg_state	"Current state of the OTG Host FSM.
>3'b000 : H_IDLE
>3'b001 : H_VBUS_ON
>3'b010 : H_VBUS_FAILED
>3'b011 : H_OTG_HOST_MODE
>3'b100 : H_HOST_MODE
>3'b101 : H_SWITCH_TO_DEVICE
>3'b110 : H_A_SUSPEND
>3'b111 : H_WAIT_VBUS_FALL"	RO	0x0
>2:0	dev_otg_state	"Current state of the OTG Device FSM.
>3'b000 : DEV_IDLE
>3'b001 : DEV_MODE
>3'b010 : DEV_SRP
>3'b011 : DEV_WAIT_VBUS_FALL
>3'b100 : DEV_SWITCH_TO_HOST
>3'b101 : DEV_WAIT_FOR_CONN"
>
>>
>>CDNS3_ROLE_GADGET: gadget mode and VBUS on
>>CDNS3_ROLE_HOST: host mode and VBUS on
>>CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
>>
>>So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
>>and need to set role as CDNS3_ROLE_END at ->stop for further handling at
>>role switch routine.
>>
>
>>> > +     mutex_unlock(&cdns->mutex);
>>> > +}
>>> > +
>>> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>> > +{
>>> > +     if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>>> > +             //TODO: implements selecting device/host mode
>>> > +             return CDNS3_ROLE_HOST;
>>> > +     }
>>> > +     return cdns->roles[CDNS3_ROLE_HOST]
>>> > +             ? CDNS3_ROLE_HOST
>>> > +             : CDNS3_ROLE_GADGET;

Cheers 
Pawel

Patch

diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
index dcdd62003c6a..02d25b23c5d3 100644
--- a/drivers/usb/cdns3/Makefile
+++ b/drivers/usb/cdns3/Makefile
@@ -1,3 +1,5 @@ 
+obj-$(CONFIG_USB_CDNS3)			+= cdns3.o
 obj-$(CONFIG_USB_CDNS3_PCI_WRAP)	+= cdns3-pci.o
 
+cdns3-y					:= core.o
 cdns3-pci-y		 		:= cdns3-pci-wrap.o
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
new file mode 100644
index 000000000000..f9055d4da67f
--- /dev/null
+++ b/drivers/usb/cdns3/core.c
@@ -0,0 +1,413 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Cadence USBSS DRD Driver.
+ *
+ * Copyright (C) 2018 Cadence.
+ *
+ * Author: Peter Chen <[e-mail]>
+ *         Pawel Laszczak <[e-mail]>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+
+#include "gadget.h"
+#include "core.h"
+
+static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
+{
+	WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
+	return cdns->roles[cdns->role];
+}
+
+static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles role)
+{
+	int ret;
+
+	if (role >= CDNS3_ROLE_END)
+		return 0;
+
+	if (!cdns->roles[role])
+		return -ENXIO;
+
+	mutex_lock(&cdns->mutex);
+	cdns->role = role;
+	ret = cdns->roles[role]->start(cdns);
+	mutex_unlock(&cdns->mutex);
+	return ret;
+}
+
+static inline void cdns3_role_stop(struct cdns3 *cdns)
+{
+	enum cdns3_roles role = cdns->role;
+
+	if (role == CDNS3_ROLE_END)
+		return;
+
+	mutex_lock(&cdns->mutex);
+	cdns->roles[role]->stop(cdns);
+	cdns->role = CDNS3_ROLE_END;
+	mutex_unlock(&cdns->mutex);
+}
+
+static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
+{
+	if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
+		//TODO: implements selecting device/host mode
+		return CDNS3_ROLE_HOST;
+	}
+	return cdns->roles[CDNS3_ROLE_HOST]
+		? CDNS3_ROLE_HOST
+		: CDNS3_ROLE_GADGET;
+}
+
+/**
+ * cdns3_core_init_role - initialize role of operation
+ * @cdns: Pointer to cdns3 structure
+ *
+ * Returns 0 on success otherwise negative errno
+ */
+static int cdns3_core_init_role(struct cdns3 *cdns)
+{
+	struct device *dev = cdns->dev;
+	enum usb_dr_mode dr_mode;
+
+	dr_mode = usb_get_dr_mode(dev);
+	cdns->role = CDNS3_ROLE_END;
+
+	/*
+	 * If driver can't read mode by means of usb_get_dr_mdoe function then
+	 * chooses mode according with Kernel configuration. This setting
+	 * can be restricted later depending on strap pin configuration.
+	 */
+	if (dr_mode == USB_DR_MODE_UNKNOWN) {
+		if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
+		    IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
+			dr_mode = USB_DR_MODE_OTG;
+		else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
+			dr_mode = USB_DR_MODE_HOST;
+		else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
+			dr_mode = USB_DR_MODE_PERIPHERAL;
+	}
+
+	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
+		//TODO: implements host initialization
+	}
+
+	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
+		//TODO: implements device initialization
+	}
+
+	if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET]) {
+		dev_err(dev, "no supported roles\n");
+		return -ENODEV;
+	}
+
+	cdns->dr_mode = dr_mode;
+	return 0;
+}
+
+/**
+ * cdns3_irq - interrupt handler for cdns3 core device
+ *
+ * @irq: irq number for cdns3 core device
+ * @data: structure of cdns3
+ *
+ * Returns IRQ_HANDLED or IRQ_NONE
+ */
+static irqreturn_t cdns3_irq(int irq, void *data)
+{
+	struct cdns3 *cdns = data;
+	irqreturn_t ret = IRQ_NONE;
+
+	/* Handle device/host interrupt */
+	if (cdns->role != CDNS3_ROLE_END)
+		ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
+
+	return ret;
+}
+
+static void cdns3_remove_roles(struct cdns3 *cdns)
+{
+	//TODO: implements this function
+}
+
+static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
+{
+	enum cdns3_roles current_role;
+	int ret = 0;
+
+	current_role = cdns->role;
+
+	if (role == CDNS3_ROLE_END)
+		return 0;
+
+	dev_dbg(cdns->dev, "Switching role");
+
+	ret = cdns3_role_start(cdns, role);
+	if (ret) {
+		/* Back to current role */
+		dev_err(cdns->dev, "set %d has failed, back to %d\n",
+			role, current_role);
+		ret = cdns3_role_start(cdns, current_role);
+	}
+
+	return ret;
+}
+
+/**
+ * cdns3_role_switch - work queue handler for role switch
+ *
+ * @work: work queue item structure
+ *
+ * Handles below events:
+ * - Role switch for dual-role devices
+ * - CDNS3_ROLE_GADGET <--> CDNS3_ROLE_END for peripheral-only devices
+ */
+static void cdns3_role_switch(struct work_struct *work)
+{
+	enum cdns3_roles role = CDNS3_ROLE_END;
+	struct cdns3 *cdns;
+	bool device, host;
+
+	cdns = container_of(work, struct cdns3, role_switch_wq);
+
+	//TODO: implements this functions.
+	//host = cdns3_is_host(cdns);
+	//device = cdns3_is_device(cdns);
+	host = 1;
+	device = 0;
+
+	if (host)
+		role = CDNS3_ROLE_HOST;
+	else if (device)
+		role = CDNS3_ROLE_GADGET;
+
+	if (cdns->desired_dr_mode == cdns->current_dr_mode &&
+	    cdns->role == role)
+		return;
+
+	pm_runtime_get_sync(cdns->dev);
+	cdns3_role_stop(cdns);
+
+	if (host) {
+		if (cdns->roles[CDNS3_ROLE_HOST])
+			cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
+		pm_runtime_put_sync(cdns->dev);
+		return;
+	}
+
+	if (device)
+		cdns3_do_role_switch(cdns, CDNS3_ROLE_GADGET);
+	else
+		cdns3_do_role_switch(cdns, CDNS3_ROLE_END);
+
+	pm_runtime_put_sync(cdns->dev);
+}
+
+/**
+ * cdns3_probe - probe for cdns3 core device
+ * @pdev: Pointer to cdns3 core platform device
+ *
+ * Returns 0 on success otherwise negative errno
+ */
+static int cdns3_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource	*res;
+	struct cdns3 *cdns;
+	void __iomem *regs;
+	int ret;
+
+	cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
+	if (!cdns)
+		return -ENOMEM;
+
+	cdns->dev = dev;
+
+	platform_set_drvdata(pdev, cdns);
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(dev, "missing IRQ\n");
+		return -ENODEV;
+	}
+	cdns->irq = res->start;
+
+	/*
+	 * Request memory region
+	 * region-0: xHCI
+	 * region-1: Peripheral
+	 * region-2: OTG registers
+	 */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(dev, res);
+
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+	cdns->xhci_regs = regs;
+	cdns->xhci_res = res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+	cdns->dev_regs	= regs;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+	cdns->otg_regs = regs;
+
+	mutex_init(&cdns->mutex);
+
+	cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
+	if (IS_ERR(cdns->phy)) {
+		dev_info(dev, "no generic phy found\n");
+		cdns->phy = NULL;
+		/*
+		 * fall through here!
+		 * if no generic phy found, phy init
+		 * should be done under boot!
+		 */
+	} else {
+		phy_init(cdns->phy);
+	}
+
+	ret = cdns3_core_init_role(cdns);
+	if (ret)
+		goto err1;
+
+	INIT_WORK(&cdns->role_switch_wq, cdns3_role_switch);
+	if (ret)
+		goto err2;
+
+	if (ret)
+		goto err2;
+
+	cdns->role = cdns3_get_role(cdns);
+
+	ret = devm_request_irq(dev, cdns->irq, cdns3_irq, IRQF_SHARED,
+			       dev_name(dev), cdns);
+
+	if (ret)
+		goto err2;
+
+	ret = cdns3_role_start(cdns, cdns->role);
+	if (ret) {
+		dev_err(dev, "can't start %s role\n",
+			cdns3_get_current_role_driver(cdns)->name);
+		goto err2;
+	}
+
+	device_set_wakeup_capable(dev, true);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	/*
+	 * The controller needs less time between bus and controller suspend,
+	 * and we also needs a small delay to avoid frequently entering low
+	 * power mode.
+	 */
+	pm_runtime_set_autosuspend_delay(dev, 20);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_use_autosuspend(dev);
+	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
+
+	return 0;
+
+err2:
+	cdns3_remove_roles(cdns);
+err1:
+	return ret;
+}
+
+/**
+ * cdns3_remove - unbind drd driver and clean up
+ * @pdev: Pointer to Linux platform device
+ *
+ * Returns 0 on success otherwise negative errno
+ */
+static int cdns3_remove(struct platform_device *pdev)
+{
+	struct cdns3 *cdns = platform_get_drvdata(pdev);
+
+	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	cdns3_remove_roles(cdns);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_cdns3_match[] = {
+	{ .compatible = "cdns,usb3" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_cdns3_match);
+#endif
+
+#ifdef CONFIG_PM
+
+#ifdef CONFIG_PM_SLEEP
+static int cdns3_suspend(struct device *dev)
+{
+	//TODO: implements this function
+	return 0;
+}
+
+static int cdns3_resume(struct device *dev)
+{
+	//TODO: implements this function
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+static int cdns3_runtime_suspend(struct device *dev)
+{	//TODO: implements this function
+	return 0;
+}
+
+static int cdns3_runtime_resume(struct device *dev)
+{
+	//TODO: implements this function
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops cdns3_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
+	SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
+};
+
+static struct platform_driver cdns3_driver = {
+	.probe		= cdns3_probe,
+	.remove		= cdns3_remove,
+	.driver		= {
+		.name	= "cdns-usb3",
+		.of_match_table	= of_match_ptr(of_cdns3_match),
+		.pm	= &cdns3_pm_ops,
+	},
+};
+
+static int __init cdns3_driver_platform_register(void)
+{
+	return platform_driver_register(&cdns3_driver);
+}
+module_init(cdns3_driver_platform_register);
+
+static void __exit cdns3_driver_platform_unregister(void)
+{
+	platform_driver_unregister(&cdns3_driver);
+}
+module_exit(cdns3_driver_platform_unregister);
+
+MODULE_ALIAS("platform:cdns3");
+MODULE_AUTHOR("Pawel Laszczak <[e-mail]>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
new file mode 100644
index 000000000000..7c8204fe4d3d
--- /dev/null
+++ b/drivers/usb/cdns3/core.h
@@ -0,0 +1,100 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Cadence USBSS DRD Driver.
+ *
+ * Copyright (C) 2017 NXP
+ * Copyright (C) 2018 Cadence.
+ *
+ * Authors: Peter Chen <[e-mail]>
+ *          Pawel Laszczak <[e-mail]>
+ */
+#include <linux/usb/otg.h>
+
+#ifndef __LINUX_CDNS3_CORE_H
+#define __LINUX_CDNS3_CORE_H
+
+struct cdns3;
+enum cdns3_roles {
+	CDNS3_ROLE_HOST = 0,
+	CDNS3_ROLE_GADGET,
+	CDNS3_ROLE_END,
+};
+
+/**
+ * struct cdns3_role_driver - host/gadget role driver
+ * @start: start this role
+ * @stop: stop this role
+ * @suspend: suspend callback for this role
+ * @resume: resume callback for this role
+ * @irq: irq handler for this role
+ * @name: role name string (host/gadget)
+ */
+struct cdns3_role_driver {
+	int (*start)(struct cdns3 *cdns);
+	void (*stop)(struct cdns3 *cdns);
+	int (*suspend)(struct cdns3 *cdns, bool do_wakeup);
+	int (*resume)(struct cdns3 *cdns, bool hibernated);
+	irqreturn_t (*irq)(struct cdns3 *cdns);
+	const char *name;
+};
+
+#define CDNS3_NUM_OF_CLKS	5
+/**
+ * struct cdns3 - Representation of Cadence USB3 DRD controller.
+ * @dev: pointer to Cadence device struct
+ * @xhci_regs: pointer to base of xhci registers
+ * @xhci_res: the resource for xhci
+ * @dev_regs: pointer to base of dev registers
+ * @otg_regs: pointer to base of otg registers
+ * @irq: irq number for controller
+ * @roles: array of supported roles for this controller
+ * @role: current role
+ * @host_dev: the child host device pointer for cdns3 core
+ * @gadget_dev: the child gadget device pointer for cdns3 core
+ * @usb: phy for this controller
+ * @role_switch_wq: work queue item for role switch
+ * @in_lpm: the controller in low power mode
+ * @wakeup_int: the wakeup interrupt
+ * @mutex: the mutex for concurrent code at driver
+ * @dr_mode: supported mode of operation it can be only Host, only Device
+ *           or OTG mode that allow to switch between Device and Host mode.
+ *           This field based on hardware configuration and cant't be changed.
+ * @current_dr_role: current mode of operation when in dual-role mode
+ * @desired_dr_role: desired mode of operation when in dual-role mode.
+ *           This value can be changed during runtime.
+ *           Available options depends on  dr_mode:
+ *           dr_mode                 |  desired_dr_role and current_dr_role
+ *           ----------------------------------------------------------------
+ *           USB_DR_MODE_HOST        | only USB_DR_MODE_HOST
+ *           USB_DR_MODE_PERIPHERAL  | only USB_DR_MODE_PERIPHERAL
+ *           USB_DR_MODE_OTG         | only USB_DR_MODE_HOST
+ *           USB_DR_MODE_OTG         | only USB_DR_MODE_PERIPHERAL
+ *           USB_DR_MODE_OTG         | USB_DR_MODE_OTG
+ *
+ *           Desired_dr_role can be changed by means of debugfs.
+ * @root: debugfs root folder pointer
+ */
+struct cdns3 {
+	struct device			*dev;
+	void __iomem			*xhci_regs;
+	struct resource			*xhci_res;
+	struct cdns3_usb_regs __iomem	*dev_regs;
+	struct cdns3_otg_regs		*otg_regs;
+	int irq;
+	struct cdns3_role_driver	*roles[CDNS3_ROLE_END];
+	enum cdns3_roles		role;
+	struct device			*host_dev;
+	struct device			*gadget_dev;
+	struct phy			*phy;
+	struct work_struct		role_switch_wq;
+	int				in_lpm:1;
+	int				wakeup_int:1;
+	/* mutext used in workqueue*/
+	struct mutex			mutex;
+	enum usb_dr_mode		dr_mode;
+	enum usb_dr_mode		current_dr_mode;
+	enum usb_dr_mode		desired_dr_mode;
+	struct dentry			*root;
+};
+
+#endif /* __LINUX_CDNS3_CORE_H */