| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BYAPR07MB470984EDF9452D4E219D4DE1DDB60@BYAPR07MB4709.namprd07.prod.outlook.com>
Date: Thu, 27 Dec 2018 13:11:51 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: Peter Chen <hzpeterchen@...il.com>
CC: "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"hdegoede@...hat.com" <hdegoede@...hat.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"rogerq@...com" <rogerq@...com>,
lkml <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: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
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@...ence.com
>> + *
>> + */
>> +#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
Powered by blists - more mailing lists