RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver

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

 



HI,

>>
>> The host side of USBSS-DRD controller is compliance
>> with XHCI specification, so it works with
>> standard XHCI linux driver.
>>
>
>After adding my glue layer change (with my phy part) and make one
>change for this code,
>the xHCI can work at my platform. I list the comments from today's
>review and debug.
>The comments are at  Makefile and drd.c.
>
>> +         If you choose to build this driver as module it will
>> +         be dynamically linked and module will be called cdns3-pci.ko
>> +
>> +endif
>> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> new file mode 100644
>> index 000000000000..3f63baa24294
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/Makefile
>> @@ -0,0 +1,16 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# define_trace.h needs to know how to find our header
>> +CFLAGS_trace.o                         := -I$(src)
>> +
>> +obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
>> +obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
>> +
>> +cdns3-y                                        := core.o drd.o trace.o
>> +
>> +ifneq ($(CONFIG_DEBUG_FS),)
>> +       cdns3-y                         += debugfs.o
>> +endif
>> +
>> +cdns3-$(CONFIG_USB_CDNS3_GADGET)       += gadget.o ep0.o
>> +cdns3-$(CONFIG_USB_CDNS3_HOST)         += host.o
>> +cdns3-pci-y                            := cdns3-pci-wrap.o
>
>Why do you want add two lines for pci_wrap? I change this Makefile like below:

I don't need these two lines. I will change it as you suggested. 

>
># SPDX-License-Identifier: GPL-2.0
># define_trace.h needs to know how to find our header
>CFLAGS_trace.o                          := -I$(src)
>
>cdns3-y                                 := core.o drd.o trace.o
>obj-$(CONFIG_USB_CDNS3)                 += cdns3.o
>ifneq ($(CONFIG_DEBUG_FS),)
>        cdns3-y                         += debugfs.o
>endif
>
>cdns3-$(CONFIG_USB_CDNS3_GADGET)        += gadget.o ep0.o
>cdns3-$(CONFIG_USB_CDNS3_HOST)          += host.o
>obj-$(CONFIG_USB_CDNS3_PCI_WRAP)        += cdns3-pci-wrap.o
>obj-$(CONFIG_USB_CDNS3_IMX_WRAP)        += cdns3-imx.o
>
>and below is the diff:
>
>diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>index 3f63baa24294..d1bca2829f57 100644
>--- a/drivers/usb/cdns3/Makefile
>+++ b/drivers/usb/cdns3/Makefile
>@@ -2,15 +2,13 @@
> # define_trace.h needs to know how to find our header
> CFLAGS_trace.o                         := -I$(src)
>
>-obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
>-obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci.o
>-
> cdns3-y                                        := core.o drd.o trace.o
>-
>+obj-$(CONFIG_USB_CDNS3)                        += cdns3.o
> ifneq ($(CONFIG_DEBUG_FS),)
>        cdns3-y                         += debugfs.o
> endif
>
> cdns3-$(CONFIG_USB_CDNS3_GADGET)       += gadget.o ep0.o
> cdns3-$(CONFIG_USB_CDNS3_HOST)         += host.o
>-cdns3-pci-y                            := cdns3-pci-wrap.o
>+obj-$(CONFIG_USB_CDNS3_PCI_WRAP)       += cdns3-pci-wrap.o
>+obj-$(CONFIG_USB_CDNS3_IMX_WRAP)       += cdns3-imx.o
>
>
>> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
>> new file mode 100644
>> index 000000000000..b0c32302eb0b
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/drd.c
>> @@ -0,0 +1,350 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver.
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak <pawell@xxxxxxxxxxx
>> + *
>> + */
>> +#include <linux/kernel.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
>> +#include <linux/usb/otg.h>
>> +
>> +#include "gadget.h"
>> +#include "drd.h"
>> +#include "core.h"
>> +
>> +static int cdns3_drd_switch_gadget(struct cdns3 *cdns, int on);
>> +static int cdns3_drd_switch_host(struct cdns3 *cdns, int on);
>> +
>> +/**
>> + * cdns3_set_mode - change mode of OTG Core
>> + * @cdns: pointer to context structure
>> + * @mode: selected mode from cdns_role
>> + */
>> +void cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
>> +{
>> +       u32 reg;
>> +
>> +       cdns->current_dr_mode = mode;
>> +
>> +       switch (mode) {
>> +       case USB_DR_MODE_PERIPHERAL:
>> +               dev_info(cdns->dev, "Set controller to Gadget mode\n");
>> +               cdns3_drd_switch_gadget(cdns, 1);
>> +               break;
>> +       case USB_DR_MODE_HOST:
>> +               dev_info(cdns->dev, "Set controller to Host mode\n");
>> +               cdns3_drd_switch_host(cdns, 1);
>> +               break;
>> +       case USB_DR_MODE_OTG:
>> +               dev_info(cdns->dev, "Set controller to OTG mode\n");
>> +               if (cdns->version == CDNS3_CONTROLLER_V1) {
>> +                       reg = readl(&cdns->otg_v1_regs->override);
>> +                       reg |= OVERRIDE_IDPULLUP;
>> +                       writel(reg, &cdns->otg_v1_regs->override);
>> +               } else {
>> +                       reg = readl(&cdns->otg_v0_regs->ctrl1);
>> +                       reg |= OVERRIDE_IDPULLUP_V0;
>> +                       writel(reg, &cdns->otg_v0_regs->ctrl1);
>> +               }
>> +
>> +               /*
>> +                * Hardware specification says: "ID_VALUE must be valid within
>> +                * 50ms after idpullup is set to '1" so driver must wait
>> +                * 50ms before reading this pin.
>> +                */
>> +               usleep_range(50000, 60000);
>> +               break;
>> +       default:
>> +               cdns->current_dr_mode = USB_DR_MODE_UNKNOWN;
>> +               dev_err(cdns->dev, "Unsupported mode of operation %d\n", mode);
>> +               return;
>> +       }
>> +}
>
>I suggest adding return value since switch may fail.
>
>
>> +/**
>> + * cdns3_init_otg_mode - initialize drd controller
>> + * @cdns: Pointer to controller context structure
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static void cdns3_init_otg_mode(struct cdns3 *cdns)
>
>You may add return value for this function too like your comment.
Ok, I will do it.
>
>> +{
>> +       cdns3_otg_disable_irq(cdns);
>> +       /* clear all interrupts */
>> +       writel(~0, &cdns->otg_regs->ivect);
>> +
>> +       cdns3_set_mode(cdns, USB_DR_MODE_OTG);
>
>return value
>> +
>> +       if (cdns3_is_host(cdns))
>> +               cdns3_drd_switch_host(cdns, 1);
>
>ditto
>
>> +       else
>> +               cdns3_drd_switch_gadget(cdns, 1);
>
>ditto
>> +
>> +       cdns3_otg_enable_irq(cdns);
>> +}
>> +
>> +/**
>> + * cdns3_drd_update_mode - initialize mode of operation
>> + * @cdns: Pointer to controller context structure
>> + *
>> + * 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;
>> +
>> +       cdns3_drd_switch_gadget(cdns, 0);
>
>return value
>
>> +       cdns3_drd_switch_host(cdns, 0);
	In these two line we don't need check return value.
	For disabling mode these functions always return 0. 

>ditto
>> +
>> +       switch (cdns->desired_dr_mode) {
>> +       case USB_DR_MODE_PERIPHERAL:
>> +               cdns3_set_mode(cdns, USB_DR_MODE_PERIPHERAL);
>
>ditto
>
>> +               break;
>> +       case USB_DR_MODE_HOST:
>> +               cdns3_set_mode(cdns, USB_DR_MODE_HOST);
>
>ditto
>> +               break;
>> +       case USB_DR_MODE_OTG:
>> +               cdns3_init_otg_mode(cdns);
>
>ditto
>> +               break;
>> +       default:
>> +               dev_err(cdns->dev, "Unsupported mode of operation %d\n",
>> +                       cdns->dr_mode);
>> +               return -EINVAL;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>> + * cdns3_drd_irq - interrupt handler for OTG events
>> + *
>> + * @irq: irq number for cdns3 core device
>> + * @data: structure of cdns3
>> + *
>> + * Returns IRQ_HANDLED or IRQ_NONE
>> + */
>> +static irqreturn_t cdns3_drd_irq(int irq, void *data)
>> +{
>> +       irqreturn_t ret = IRQ_NONE;
>> +       struct cdns3 *cdns = data;
>> +       u32 reg;
>> +
>> +       if (cdns->dr_mode != USB_DR_MODE_OTG)
>> +               return ret;
>> +
>> +       reg = readl(&cdns->otg_regs->ivect);
>> +
>> +       if (!reg)
>> +               return ret;
>> +
>> +       if (reg & OTGIEN_ID_CHANGE_INT) {
>> +               dev_dbg(cdns->dev, "OTG IRQ: new ID: %d\n",
>> +                       cdns3_get_id(cdns));
>> +
>> +               queue_work(system_freezable_wq, &cdns->role_switch_wq);
>> +
>> +               ret = IRQ_HANDLED;
>> +       }
>> +
>> +       writel(~0, &cdns->otg_regs->ivect);
>> +       return ret;
>> +}
>> +
>> +int cdns3_drd_init(struct cdns3 *cdns)
>> +{
>> +       void __iomem *regs;
>> +       int ret = 0;
>> +       u32 state;
>> +
>> +       regs = devm_ioremap_resource(cdns->dev, &cdns->otg_res);
>> +       if (IS_ERR(regs))
>> +               return PTR_ERR(regs);
>> +
>> +       /* Detection of DRD version. Controller has been released
>> +        * in two versions. Both are similar, but they have same changes
>> +        * in register maps.
>> +        * The first register in old version is command register and it's read
>> +        * only, so driver should read 0 from it. On the other hand, in v1
>> +        * the first register contains device ID number which is not set to 0.
>> +        * Driver uses this fact to detect the proper version of
>> +        * controller.
>> +        */
>> +       cdns->otg_v0_regs = regs;
>> +       if (!readl(&cdns->otg_v0_regs->cmd)) {
>> +               cdns->version  = CDNS3_CONTROLLER_V0;
>> +               cdns->otg_v1_regs = NULL;
>> +               cdns->otg_regs = regs;
>> +               dev_info(cdns->dev, "DRD version v0 (%08x)\n",
>> +                        readl(&cdns->otg_v0_regs->version));
>> +       } else {
>> +               cdns->otg_v0_regs = NULL;
>> +               cdns->otg_v1_regs = regs;
>> +               cdns->otg_regs = (void *)&cdns->otg_v1_regs->cmd;
>> +               cdns->version  = CDNS3_CONTROLLER_V1;
>> +               dev_info(cdns->dev, "DRD version v1 (ID: %08x, rev: %08x)\n",
>> +                        readl(&cdns->otg_v1_regs->did),
>> +                        readl(&cdns->otg_v1_regs->rid));
>> +       }
>> +
>> +       state = OTGSTS_STRAP(readl(&cdns->otg_regs->sts));
>> +
>> +       /* Update dr_mode according to STRAP configuration. */
>> +       cdns->dr_mode = USB_DR_MODE_OTG;
>> +       if (state == OTGSTS_STRAP_HOST) {
>> +               dev_info(cdns->dev, "Controller strapped to HOST\n");
>> +               cdns->dr_mode = USB_DR_MODE_HOST;
>> +       } else if (state == OTGSTS_STRAP_GADGET) {
>> +               dev_info(cdns->dev, "Controller strapped to PERIPHERAL\n");
>> +               cdns->dr_mode = USB_DR_MODE_PERIPHERAL;
>> +       }
>> +
>> +       cdns->desired_dr_mode = cdns->dr_mode;
>> +       cdns->current_dr_mode = USB_DR_MODE_UNKNOWN;
>> +
>> +       ret = devm_request_threaded_irq(cdns->dev, cdns->irq, cdns3_drd_irq,
>> +                                       NULL, IRQF_SHARED,
>> +                                       dev_name(cdns->dev), cdns);
>> +
>> +       if (ret)
>> +               return ret;
>> +
>> +       state = readl(&cdns->otg_regs->sts);
>> +       if (OTGSTS_OTG_NRDY(state) != 0) {
>> +               dev_err(cdns->dev, "Cadence USB3 OTG device not ready\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       ret = cdns3_drd_update_mode(cdns);
>> +
>
>Calling this function, it is timeout for waiting OTGSTS_XHCI_READY at otgsts,
>do you know possible reasons?  After commenting out this function, my
>xHCI function
>works.
>
The prorblem is in:
		writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
		       OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
		       &cdns->otg_regs->cmd);

After disabling current mode driver must  wait 1s before turning on  new mode. 

For simulation this time can be shortened to 2-3 ms by mans of OTGSIMULATE register. 

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