Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
From: Peter Chen
Date: Tue Dec 04 2018 - 03:50:42 EST
> > + * Cadence USBSS DRD Driver.
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Peter Chen <peter.chen@xxxxxxx>
> > + * Pawel Laszczak <pawell@xxxxxxxxxxx>
> > + */
> > +
> > +#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 <pawell@xxxxxxxxxxx>");
> > +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@xxxxxxx>
> > + * Pawel Laszczak <pawell@xxxxxxxxxxx>
> > + */
> > +#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