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

[Pawel Laszczak <pawell@cadence.com>]

>>
>> 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@cadence.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.
>> + *
>
>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 <peter.chen@nxp.com>
>> + *         Pawel Laszczak <pawell@cadence.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];
>> +}
>> +
>
>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 <pawell@cadence.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.
>> + *
>
>Header file
I don't understand. What is wrong ?
>
>> + * Copyright (C) 2017 NXP
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Authors: Peter Chen <peter.chen@nxp.com>
>> + *          Pawel Laszczak <pawell@cadence.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.
>
>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