Message-ID: <5BF7E5E8.3090406@ti.com>
Date:   Fri, 23 Nov 2018 13:35:04 +0200
From:   Roger Quadros <rogerq@...com>
To:     Pawel Laszczak <pawell@...ence.com>, <devicetree@...r.kernel.org>
CC:     <gregkh@...uxfoundation.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <adouglas@...ence.com>,
        <jbergsagel@...com>, <nsekhar@...com>, <nm@...com>,
        <sureshp@...ence.com>, <peter.chen@....com>, <pjez@...ence.com>,
        <kurahul@...ence.com>
Subject: Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.

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 <pawell@...ence.com>
> ---
>  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 <peter.chen@....com>
> + *         Pawel Laszczak <pawell@...ence.com>
> + */
> +
> +#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 <pawell@...ence.com>");
> +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 <peter.chen@....com>
> + *          Pawel Laszczak <pawell@...ence.com>
> + */
> +#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

Powered by blists - more mailing lists