Date:   Mon, 26 Nov 2018 10:17:38 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Roger Quadros <rogerq@...com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alan Douglas <adouglas@...ence.com>,
        "jbergsagel@...com" <jbergsagel@...com>,
        "nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
        Suresh Punnoose <sureshp@...ence.com>,
        "peter.chen@....com" <peter.chen@....com>,
        Pawel Jez <pjez@...ence.com>, Rahul Kumar <kurahul@...ence.com>
Subject: RE: [RFC PATCH v2 06/15] usb:cdns3: Adds Host support

>
>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@...ence.com>
>>>> ---
>>>>  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@....com>
>>>> + */
>>>> +#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@....com>
>>>> + *	    Pawel Laszczak <pawell@...ence.com>
>>>> + */
>>>> +
>>>> +#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

Powered by blists - more mailing lists