lkml.org 
[lkml]   [2019]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v10 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
    Date
    Hi,

    >
    >On 11/08/2019 14:59, Pawel Laszczak wrote:
    >> Hi,
    >>
    >>>
    >>> On 21/07/2019 21:32, Pawel Laszczak wrote:
    >>>> This patch introduce new Cadence USBSS DRD driver to Linux kernel.
    >>>>
    >>>> The Cadence USBSS DRD Controller is a highly configurable IP Core which
    >>>> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
    >>>> Host Only (XHCI)configurations.
    >>>>
    >>>> The current driver has been validated with FPGA platform. We have
    >>>> support for PCIe bus, which is used on FPGA prototyping.
    >>>>
    >>>> The host side of USBSS-DRD controller is compliant with XHCI
    >>>> specification, so it works with standard XHCI Linux driver.
    >>>>
    >>>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
    >>>> ---
    >>>> drivers/usb/Kconfig | 2 +
    >>>> drivers/usb/Makefile | 2 +
    >>>> drivers/usb/cdns3/Kconfig | 46 +
    >>>> drivers/usb/cdns3/Makefile | 17 +
    >>>> drivers/usb/cdns3/cdns3-pci-wrap.c | 203 +++
    >>>> drivers/usb/cdns3/core.c | 554 +++++++
    >>>> drivers/usb/cdns3/core.h | 109 ++
    >>>> drivers/usb/cdns3/debug.h | 171 ++
    >>>> drivers/usb/cdns3/debugfs.c | 87 ++
    >>>> drivers/usb/cdns3/drd.c | 390 +++++
    >>>> drivers/usb/cdns3/drd.h | 166 ++
    >>>> drivers/usb/cdns3/ep0.c | 914 +++++++++++
    >>>> drivers/usb/cdns3/gadget-export.h | 28 +
    >>>> drivers/usb/cdns3/gadget.c | 2338 ++++++++++++++++++++++++++++
    >>>> drivers/usb/cdns3/gadget.h | 1321 ++++++++++++++++
    >>>> drivers/usb/cdns3/host-export.h | 28 +
    >>>> drivers/usb/cdns3/host.c | 71 +
    >>>> drivers/usb/cdns3/trace.c | 11 +
    >>>> drivers/usb/cdns3/trace.h | 493 ++++++
    >>>> 19 files changed, 6951 insertions(+)
    >>>> create mode 100644 drivers/usb/cdns3/Kconfig
    >>>> create mode 100644 drivers/usb/cdns3/Makefile
    >>>> create mode 100644 drivers/usb/cdns3/cdns3-pci-wrap.c
    >>>> create mode 100644 drivers/usb/cdns3/core.c
    >>>> create mode 100644 drivers/usb/cdns3/core.h
    >>>> create mode 100644 drivers/usb/cdns3/debug.h
    >>>> create mode 100644 drivers/usb/cdns3/debugfs.c
    >>>> create mode 100644 drivers/usb/cdns3/drd.c
    >>>> create mode 100644 drivers/usb/cdns3/drd.h
    >>>> create mode 100644 drivers/usb/cdns3/ep0.c
    >>>> create mode 100644 drivers/usb/cdns3/gadget-export.h
    >>>> create mode 100644 drivers/usb/cdns3/gadget.c
    >>>> create mode 100644 drivers/usb/cdns3/gadget.h
    >>>> create mode 100644 drivers/usb/cdns3/host-export.h
    >>>> create mode 100644 drivers/usb/cdns3/host.c
    >>>> create mode 100644 drivers/usb/cdns3/trace.c
    >>>> create mode 100644 drivers/usb/cdns3/trace.h
    >>>>
    >>>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
    >>>> index e4b27413f528..c2e78882f8c2 100644
    >>>> --- a/drivers/usb/Kconfig
    >>>> +++ b/drivers/usb/Kconfig
    >>>> @@ -113,6 +113,8 @@ source "drivers/usb/usbip/Kconfig"
    >>>>
    >>>> endif
    >>>>

    <snip>

    >>>> + real_role = cdsn3_real_role_switch_get(cdns->dev);
    >>>> +
    >>>> + current_role = role;
    >>>> + dev_dbg(cdns->dev, "Switching role");
    >>>> +
    >>>> + ret = cdns3_role_start(cdns, real_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);
    >>>> + if (ret)
    >>>> + dev_err(cdns->dev, "back to %d failed too\n",
    >>>> + current_role);
    >>>> + }
    >>>> +exit:
    >>>> + pm_runtime_put_sync(cdns->dev);
    >>>> + return ret;
    >>>> +}
    >>>> +
    >>>> +static const struct usb_role_switch_desc cdns3_switch_desc = {
    >>>> + .set = cdns3_role_switch_set,
    >>>> + .get = cdsn3_real_role_switch_get,
    >>>> + .allow_userspace_control = true,
    >>>
    >>> how does user initiated cdns3_role_switch_set() via sysfs co-exist with role
    >>> changes done by hardware events. e.g. ID/VBUS?
    >>>
    >>
    >> Do you expect any issues whit this, have you seen any problem with this
    >> on your platform ?
    >>
    >> I assume that it should work in this way:
    >> 1. user change role by sysfs
    >> 2. Driver change the role according with user request.
    >> 3. If we receive correct ID/VBUS then role should not be changed
    >> because new role is the same as current set in point 2.
    >>
    >
    >I have not tested this series yet.
    >My understanding is that if user sets role to "host" or "device" then it should
    >remain in that role irrespective of ID/VBUS. Once user sets it to "none" then
    >port should set role based on ID/VBUS.

    According with your understanding it works the same way as by debugfs.
    Now I have no doubts to remove debugfs.c file :)

    >
    >>
    >>>> +};
    >>>> +
    >>>> +/**
    >>>> + * 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)

    <snip>

    >>>> + * Returns 0 on success otherwise negative errno
    >>>> + */
    >>>> +int cdns3_drd_update_mode(struct cdns3 *cdns)
    >>>> +{
    >>>> + int ret = 0;
    >>>> +
    >>>> + if (cdns->desired_dr_mode == cdns->current_dr_mode)
    >>>> + return ret;
    >>>> +
    >>>> + switch (cdns->desired_dr_mode) {
    >>>> + case USB_DR_MODE_PERIPHERAL:
    >>>> + ret = cdns3_set_mode(cdns, USB_DR_MODE_PERIPHERAL);
    >>>> + break;
    >>>> + case USB_DR_MODE_HOST:
    >>>> + ret = cdns3_set_mode(cdns, USB_DR_MODE_HOST);
    >>>> + break;
    >>>> + case USB_DR_MODE_OTG:
    >>>> + ret = cdns3_init_otg_mode(cdns);
    >>>> + break;
    >>>> + default:
    >>>> + dev_err(cdns->dev, "Unsupported mode of operation %d\n",
    >>>> + cdns->dr_mode);
    >>>> + return -EINVAL;
    >>>> + }
    >>>> +
    >>>> + return ret;
    >>>> +}
    >>>> +
    >>>> +static irqreturn_t cdns3_drd_thread_irq(int irq, void *data)
    >>>> +{
    >>>> + struct cdns3 *cdns = data;
    >>>> +
    >>>> + usb_role_switch_set_role(cdns->role_sw, cdns->role);
    >>>
    >>> How to ensure that HW events don't step over user chosen role?
    >>
    >> I need to think about this and find out how to test it and eventually force
    >> such cases.
    >>
    >> But I assume that after attaching/detaching plug the user chosen role can
    >> be forgotten.
    >>
    >
    >No. Only when user sets role to none then role should be based on HW.

    Ok, I will try to test it in next week.

    >>>
    >
    ><snip>

    Cheers,
    Pawell
    \
     
     \ /
      Last update: 2019-08-12 11:44    [W:6.803 / U:0.984 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site