RE: [RFC PATCH v2 06/15] usb:cdns3: Adds Host support

From: Pawel Laszczak
Date: Mon Nov 26 2018 - 05:17:53 EST

Next message: David Laight: "RE: [PATCH v4 2/5] nds32: Support FP emulation"
Previous message: Lothar WaÃmann: "Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state"
In reply to: Roger Quadros: "Re: [RFC PATCH v2 06/15] usb:cdns3: Adds Host support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>
>Hi,
>
>On 26/11/18 10:24, Pawel Laszczak wrote:
>>> EXTERNAL MAIL
>>>
>>>
>>> On 18/11/18 12:09, Pawel Laszczak wrote:
>>>> Patch adds host-export.h and host.c file and mplements functions that
>>>> allow to initialize, start and stop XHCI host driver.
>>>>
>>>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>>>> ---
>>>> drivers/usb/cdns3/Kconfig | 10 ++
>>>> drivers/usb/cdns3/Makefile | 1 +
>>>> drivers/usb/cdns3/core.c | 7 +-
>>>> drivers/usb/cdns3/host-export.h | 30 ++++
>>>> drivers/usb/cdns3/host.c | 256 ++++++++++++++++++++++++++++++++
>>>> 5 files changed, 302 insertions(+), 2 deletions(-)
>>>> create mode 100644 drivers/usb/cdns3/host-export.h
>>>> create mode 100644 drivers/usb/cdns3/host.c
>>>>
>>>> diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
>>>> index eb22a8692991..d92bc3d68eb0 100644
>>>> --- a/drivers/usb/cdns3/Kconfig
>>>> +++ b/drivers/usb/cdns3/Kconfig
>>>> @@ -10,6 +10,16 @@ config USB_CDNS3
>>>>
>>>> if USB_CDNS3
>>>>
>>>> +config USB_CDNS3_HOST
>>>> + bool "Cadence USB3 host controller"
>>>> + depends on USB_XHCI_HCD
>>>> + help
>>>> + Say Y here to enable host controller functionality of the
>>>> + cadence driver.
>>>> +
>>>> + Host controller is compliance with XHCI so it will use
>>>> + standard XHCI driver.
>>>> +
>>>> config USB_CDNS3_PCI_WRAP
>>>> tristate "PCIe-based Platforms"
>>>> depends on USB_PCI && ACPI
>>>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>>>> index e779b2a2f8eb..976117ba67ff 100644
>>>> --- a/drivers/usb/cdns3/Makefile
>>>> +++ b/drivers/usb/cdns3/Makefile
>>>> @@ -2,4 +2,5 @@ obj-$(CONFIG_USB_CDNS3) += cdns3.o
>>>> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>>>>
>>>> cdns3-y := core.o drd.o
>>>> +cdns3-$(CONFIG_USB_CDNS3_HOST) += host.o
>>>> cdns3-pci-y := cdns3-pci-wrap.o
>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>> index dbee4325da7f..4cb820be9ff3 100644
>>>> --- a/drivers/usb/cdns3/core.c
>>>> +++ b/drivers/usb/cdns3/core.c
>>>> @@ -17,6 +17,7 @@
>>>>
>>>> #include "gadget.h"
>>>> #include "core.h"
>>>> +#include "host-export.h"
>>>> #include "drd.h"
>>>>
>>>> static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>>> @@ -98,7 +99,8 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>> }
>>>>
>>>> if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>> - //TODO: implements host initialization
>>>> + if (cdns3_host_init(cdns))
>>>> + dev_info(dev, "doesn't support host\n");
>>>
>>> dev_err()
>>>
>>> And you need to error out with error code.
>>
>> ok, but I assume that even if host returns error then we can use
>> only device role. Only when both functions return errors, then it's a critical error
>> and function return error code.
>
>But at this point we are in OTG or HOST dr_mode and without host functional
>both will not function correctly. So we must error out so user can debug.

Ok,

>
>>>
>>>> }
>>>>
>>>> if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>>>> @@ -142,7 +144,7 @@ static irqreturn_t cdns3_irq(int irq, void *data)
>>>>
>>>> static void cdns3_remove_roles(struct cdns3 *cdns)
>>>> {
>>>> - //TODO: implements this function
>>>
>>> if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST)
>>>
>>>> + cdns3_host_remove(cdns);
>>>
>>> How about calling it cdns3_host_exit() to complement cdns3_host_init().
>>>
>>>> }
>>>>
>>>> static int cdns3_do_role_switch(struct cdns3 *cdns, enum cdns3_roles role)
>>>> @@ -410,6 +412,7 @@ static struct platform_driver cdns3_driver = {
>>>>
>>>> static int __init cdns3_driver_platform_register(void)
>>>> {
>>>> + cdns3_host_driver_init();
>>>> return platform_driver_register(&cdns3_driver);
>>>> }
>>>> module_init(cdns3_driver_platform_register);
>>>> diff --git a/drivers/usb/cdns3/host-export.h b/drivers/usb/cdns3/host-export.h
>>>> new file mode 100644
>>>> index 000000000000..f8f3b230b472
>>>> --- /dev/null
>>>> +++ b/drivers/usb/cdns3/host-export.h
>>>> @@ -0,0 +1,30 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Cadence USBSS DRD Driver -Host Export APIs
>>>> + *
>>>> + * Copyright (C) 2017 NXP
>>>> + *
>>>> + * Authors: Peter Chen <peter.chen@xxxxxxx>
>>>> + */
>>>> +#ifndef __LINUX_CDNS3_HOST_EXPORT
>>>> +#define __LINUX_CDNS3_HOST_EXPORT
>>>> +
>>>> +#ifdef CONFIG_USB_CDNS3_HOST
>>>> +
>>>> +int cdns3_host_init(struct cdns3 *cdns);
>>>> +void cdns3_host_remove(struct cdns3 *cdns);
>>>> +void cdns3_host_driver_init(void);
>>>> +
>>>> +#else
>>>> +
>>>> +static inline int cdns3_host_init(struct cdns3 *cdns)
>>>> +{
>>>> + return -ENXIO;
>>>> +}
>>>> +
>>>> +static inline void cdns3_host_remove(struct cdns3 *cdns) { }
>>>> +static inline void cdns3_host_driver_init(void) {}
>>>> +
>>>> +#endif /* CONFIG_USB_CDNS3_HOST */
>>>> +
>>>> +#endif /* __LINUX_CDNS3_HOST_EXPORT */
>>>> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
>>>> new file mode 100644
>>>> index 000000000000..0dd47976cb28
>>>> --- /dev/null
>>>> +++ b/drivers/usb/cdns3/host.c
>>>> @@ -0,0 +1,256 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Cadence USBSS DRD Driver - host side
>>>> + *
>>>> + * Copyright (C) 2018 Cadence Design Systems.
>>>> + * Copyright (C) 2018 NXP
>>>> + *
>>>> + * Authors: Peter Chen <peter.chen@xxxxxxx>
>>>> + * Pawel Laszczak <pawell@xxxxxxxxxxx>
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/dma-mapping.h>
>>>> +#include <linux/usb.h>
>>>> +#include <linux/usb/hcd.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/usb/of.h>
>>>> +
>>>> +#include "../host/xhci.h"
>>>> +#include "core.h"
>>>> +#include "host-export.h"
>>>> +
>>>> +static struct hc_driver __read_mostly xhci_cdns3_hc_driver;
>>>> +
>>>> +static void xhci_cdns3_quirks(struct device *dev, struct xhci_hcd *xhci)
>>>> +{
>>>> + /*
>>>> + * As of now platform drivers don't provide MSI support so we ensure
>>>> + * here that the generic code does not try to make a pci_dev from our
>>>> + * dev struct in order to setup MSI
>>>> + */
>>>> + xhci->quirks |= XHCI_PLAT;
>>>> +}
>>>> +
>>>> +static int xhci_cdns3_setup(struct usb_hcd *hcd)
>>>> +{
>>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> + u32 command;
>>>> + int ret;
>>>> +
>>>> + ret = xhci_gen_setup(hcd, xhci_cdns3_quirks);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* set usbcmd.EU3S */
>>>> + command = readl(&xhci->op_regs->command);
>>>> + command |= CMD_PM_INDEX;
>>>> + writel(command, &xhci->op_regs->command);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct xhci_driver_overrides xhci_cdns3_overrides __initconst = {
>>>> + .extra_priv_size = sizeof(struct xhci_hcd),
>>>> + .reset = xhci_cdns3_setup,
>>>> +};
>>>> +
>>>> +struct cdns3_host {
>>>> + struct device dev;
>>>> + struct usb_hcd *hcd;
>>>> +};
>>>> +
>>>> +static irqreturn_t cdns3_host_irq(struct cdns3 *cdns)
>>>> +{
>>>> + struct device *dev = cdns->host_dev;
>>>> + struct usb_hcd *hcd;
>>>> +
>>>> + if (dev)
>>>> + hcd = dev_get_drvdata(dev);
>>>> + else
>>>> + return IRQ_NONE;
>>>> +
>>>> + if (hcd)
>>>> + return usb_hcd_irq(cdns->irq, hcd);
>>>> + else
>>>> + return IRQ_NONE;
>>>
>>> Why can't you just reuse the xhci-platform driver and let it manage the IRQ?
>>> Since it is a shared IRQ, different drivers can request the same IRQ and return IRQ_NONE
>>> if the IRQ wasn't from their device.
>>
>> In device role the host part of controller is kept in reset, so driver can't read the host register.
>> Such solution allows driver to control access to host register.
>> So if driver has shared separate interrupt for host role then it has to check if controller work in
>> Host role.
>
>I understand what you mean. I think the issue here is that you are having the host ISR active
>even when host role is stopped. This is the root cause of the problem.
>
>When you stop host role, the host driver *must* unregister the ISR
>and then place the host in reset.
>
>This will happen correctly if you use platform_unregister_device() to unregister the
>XHCI device in cdns3_host_stop().

Ok, now I understood you concept. I will test it.
>>
>>>> +}
>>>> +
>>>> +static void cdns3_host_release(struct device *dev)
>>>> +{
>>>> + struct cdns3_host *host = container_of(dev, struct cdns3_host, dev);
>>>> +
>>>> + kfree(host);
>>>> +}
>>>> +
>>>> +static int cdns3_host_start(struct cdns3 *cdns)
>>>> +{
>>>> + struct cdns3_host *host;
>>>> + struct device *dev;
>>>> + struct device *sysdev;
>>>> + struct xhci_hcd *xhci;
>>>> + int ret;
>>>> +
>>>> + host = kzalloc(sizeof(*host), GFP_KERNEL);
>>>> + if (!host)
>>>> + return -ENOMEM;
>>>> +
>>>> + dev = &host->dev;
>>>> + dev->release = cdns3_host_release;
>>>> + dev->parent = cdns->dev;
>>>> + dev_set_name(dev, "xhci-cdns3");
>>>> + cdns->host_dev = dev;
>>>> + ret = device_register(dev);
>>>> + if (ret)
>>>> + goto err1;
>>>> +
>>>> + sysdev = cdns->dev;
>>>> + /* Try to set 64-bit DMA first */
>>>> + if (WARN_ON(!sysdev->dma_mask))
>>>> + /* Platform did not initialize dma_mask */
>>>> + ret = dma_coerce_mask_and_coherent(sysdev,
>>>> + DMA_BIT_MASK(64));
>>>> + else
>>>> + ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
>>>> +
>>>> + /* If setting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
>>>> + if (ret) {
>>>> + ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(32));
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + pm_runtime_set_active(dev);
>>>> + pm_runtime_no_callbacks(dev);
>>>> + pm_runtime_enable(dev);
>>>> + host->hcd = __usb_create_hcd(&xhci_cdns3_hc_driver, sysdev, dev,
>>>> + dev_name(dev), NULL);
>>>> + if (!host->hcd) {
>>>> + ret = -ENOMEM;
>>>> + goto err2;
>>>> + }
>>>> +
>>>> + host->hcd->regs = cdns->xhci_regs;
>>>> + host->hcd->rsrc_start = cdns->xhci_res->start;
>>>> + host->hcd->rsrc_len = resource_size(cdns->xhci_res);
>>>> +
>>>> + device_wakeup_enable(host->hcd->self.controller);
>>>> + xhci = hcd_to_xhci(host->hcd);
>>>> +
>>>> + xhci->main_hcd = host->hcd;
>>>> + xhci->shared_hcd = __usb_create_hcd(&xhci_cdns3_hc_driver, sysdev, dev,
>>>> + dev_name(dev), host->hcd);
>>>> + if (!xhci->shared_hcd) {
>>>> + ret = -ENOMEM;
>>>> + goto err3;
>>>> + }
>>>> +
>>>> + host->hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
>>>> + xhci->shared_hcd->tpl_support = host->hcd->tpl_support;
>>>> + ret = usb_add_hcd(host->hcd, 0, IRQF_SHARED);
>>>> + if (ret)
>>>> + goto err4;
>>>> +
>>>> + ret = usb_add_hcd(xhci->shared_hcd, 0, IRQF_SHARED);
>>>> + if (ret)
>>>> + goto err5;
>>>> +
>>>> + device_set_wakeup_capable(dev, true);
>>>
>>> All this is being done by the xhci-plat.c
>>>
>>> You can make use of it by just creating a xhci-hcd platform device.
>>>
>>> e.g.
>>> platform_device_alloc("xhci-hcd", PLATFORM_DEVID_AUTO);
>>> platform_device_add_resources() to add IRQ and memory resource.
>>> platform_device_add_properties() to add any quirks.
>>> platform_device_add()
>>>
>>
>> If we do this in this way driver will not control the interrupt.
>
>Why should this driver control host interrupt when it doesn't
>have access to HOST registers.
>

You you are right. I doesn't have to.

>> This code has written by Peter Chan and I am convinced
>> that this concept is only correct one.
>>
>>>> +
>>>> + return 0;
>>>> +
>>>> +err5:
>>>> + usb_remove_hcd(host->hcd);
>>>> +err4:
>>>> + usb_put_hcd(xhci->shared_hcd);
>>>> +err3:
>>>> + usb_put_hcd(host->hcd);
>>>> +err2:
>>>> + device_del(dev);
>>>> +err1:
>>>> + put_device(dev);
>>>> + cdns->host_dev = NULL;
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void cdns3_host_stop(struct cdns3 *cdns)
>>>> +{
>>>> + struct device *dev = cdns->host_dev;
>>>> + struct xhci_hcd *xhci;
>>>> + struct usb_hcd *hcd;
>>>> +
>>>> + if (dev) {
>>>> + hcd = dev_get_drvdata(dev);
>>>> + xhci = hcd_to_xhci(hcd);
>>>> + usb_remove_hcd(xhci->shared_hcd);
>>>> + usb_remove_hcd(hcd);
>>>> + synchronize_irq(cdns->irq);
>>>> + usb_put_hcd(xhci->shared_hcd);
>>>> + usb_put_hcd(hcd);
>>>> + cdns->host_dev = NULL;
>>>> + pm_runtime_set_suspended(dev);
>>>> + pm_runtime_disable(dev);
>>>> + device_del(dev);
>>>> + put_device(dev);
>>>> + }
>>>
>>> You can replace this with just
>>> platform_device_unregister(xhci_dev);
>>>
>>>> +}
>>>> +
>>>> +#if CONFIG_PM
>>>> +static int cdns3_host_suspend(struct cdns3 *cdns, bool do_wakeup)
>>>> +{
>>>> + struct device *dev = cdns->host_dev;
>>>> + struct xhci_hcd *xhci;
>>>> +
>>>> + if (!dev)
>>>> + return 0;
>>>> +
>>>> + xhci = hcd_to_xhci(dev_get_drvdata(dev));
>>>> + return xhci_suspend(xhci, do_wakeup);
>>>> +}
>>>> +
>>>> +static int cdns3_host_resume(struct cdns3 *cdns, bool hibernated)
>>>> +{
>>>> + struct device *dev = cdns->host_dev;
>>>> + struct xhci_hcd *xhci;
>>>> +
>>>> + if (!dev)
>>>> + return 0;
>>>> +
>>>> + xhci = hcd_to_xhci(dev_get_drvdata(dev));
>>>> + return xhci_resume(xhci, hibernated);
>>>> +}
>>>
>>> These won't be required any more as xhci-plat is doing this.
>>>
>>>> +#endif /* CONFIG_PM */
>>>> +
>>>> +int cdns3_host_init(struct cdns3 *cdns)
>>>> +{
>>>> + struct cdns3_role_driver *rdrv;
>>>> +
>>>> + rdrv = devm_kzalloc(cdns->dev, sizeof(*rdrv), GFP_KERNEL);
>>>> + if (!rdrv)
>>>> + return -ENOMEM;
>>>> +
>>>> + rdrv->start = cdns3_host_start;
>>>> + rdrv->stop = cdns3_host_stop;
>>>> + rdrv->irq = cdns3_host_irq;
>>>> +#if CONFIG_PM
>>>> + rdrv->suspend = cdns3_host_suspend;
>>>> + rdrv->resume = cdns3_host_resume;
>>>> +#endif /* CONFIG_PM */
>>>> + rdrv->name = "host";
>>>> + cdns->roles[CDNS3_ROLE_HOST] = rdrv;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +void cdns3_host_remove(struct cdns3 *cdns)
>>>> +{
>>>> + cdns3_host_stop(cdns);
>>>
>>> calling cdns3_host_stop() here can lead to problems as Controller might be in
>>> peripheral mode at this point. The core driver needs to ensure that relevant role
>>> is stopped before calling cdns3_host_remove().
>>>
>>> Here you need to unregister the role driver though.
>>>
>>> cdns->roles[CDNS3_ROLE_HOST] = NULL;
>>>
>> This function can be called only in host mode/role. It operate on host registers.
>> This checking is provided in core.c file.
>>>> +}
>>>> +
>>>> +void __init cdns3_host_driver_init(void)
>>>> +{
>>>> + xhci_init_driver(&xhci_cdns3_hc_driver, &xhci_cdns3_overrides);
>>>> +}
>>>>
>>>
>
>cheers,
>-roger
>
>--
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Next message: David Laight: "RE: [PATCH v4 2/5] nds32: Support FP emulation"
Previous message: Lothar WaÃmann: "Re: [RCF PATCH,v2,2/2] pwm: imx: Configure output to GPIO in disabled state"
In reply to: Roger Quadros: "Re: [RFC PATCH v2 06/15] usb:cdns3: Adds Host support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]