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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>
>> 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 <pawell@xxxxxxxxxxx>
>> >> ---
>> >>  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@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()?
>> 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 <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.
>> 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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux