RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi, >On Thu, Jan 10, 2019 at 09:30:41AM +0800, Peter Chen wrote: >> On Mon, Dec 24, 2018 at 12:44 AM Pawel Laszczak wrote: >> - debugfs is nice to have feature, I suggest removing it at this >> initial version. Besides, role switch >> through /sys is normal feature, the end user may use it at real >> product, so, it is better at device's >> /sys entry instead of debugfs. >> >> - I don't know why you add "disable" at debugfs, please comment. > >As you imply here, no real-world functionality should ever be in >debugfs as it is an optional system component and kernel code should >work just fine without it being enabled (as more and more systems are >disabling it due to the obvious security problems it has.) > "disable" it's not required. It's used for testing. It allows to enable/disable the current role. It can be used for testing eg. connect/disconnect event. I mainly test the driver remotely and I can't do connect/disconnect by USB cable. "disable" allow me to get trace log from host/device without disconnecting cable: I'm using the fallowing sequence: - load cdns3.ko modules - echo 1 > disable - disable current role - echo cdns3:* tracing/set_event - echo 0 > disable - enable role again - cat tracing/trace. Pawel
Re: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
On Thu, Jan 10, 2019 at 09:30:41AM +0800, Peter Chen wrote: > On Mon, Dec 24, 2018 at 12:44 AM Pawel Laszczak wrote: > - debugfs is nice to have feature, I suggest removing it at this > initial version. Besides, role switch > through /sys is normal feature, the end user may use it at real > product, so, it is better at device's > /sys entry instead of debugfs. > > - I don't know why you add "disable" at debugfs, please comment. As you imply here, no real-world functionality should ever be in debugfs as it is an optional system component and kernel code should work just fine without it being enabled (as more and more systems are disabling it due to the obvious security problems it has.) thanks, greg k-h
RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi
>On Sun, 2018-12-23 at 15:13 +, Pawel Laszczak wrote:
>> This patch introduce new Cadence USBSS DRD driver
>> to linux kernel.
>
><...>
>
>> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c
>> b/drivers/usb/cdns3/cdns3-pci-wrap.c
>> new file mode 100644
>> index ..e93179c45ece
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
>> @@ -0,0 +1,157 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS PCI Glue driver
>> + *
>> + * Copyright (C) 2018 Cadence.
>> + *
>> + * Author: Pawel Laszczak
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +struct cdns3_wrap {
>> +struct platform_device *plat_dev;
>> +struct pci_dev *hg_dev;
>> +struct resource dev_res[4];
>> +};
>> +
>> +struct cdns3_wrap wrap;
>> +
>> +#define RES_IRQ_ID 0
>> +#define RES_HOST_ID 1
>> +#define RES_DEV_ID 2
>> +#define RES_DRD_ID 3
>> +
>> +#define PCI_BAR_HOST0
>> +#define PCI_BAR_DEV 2
>> +#define PCI_BAR_OTG 4
>> +
>> +#define PCI_DEV_FN_HOST_DEVICE 0
>> +#define PCI_DEV_FN_OTG 1
>> +
>> +#define PCI_DRIVER_NAME "cdns3-pci-usbss"
>> +#define PLAT_DRIVER_NAME"cdns-usb3"
>> +
>> +#define CDNS_VENDOR_ID 0x17cd
>> +#define CDNS_DEVICE_ID 0x0100
>> +
>> +/**
>> + * cdns3_pci_probe - Probe function for Cadence USB wrapper driver
>> + * @pdev: platform device object
>> + * @id: pci device id
>> + *
>> + * Returns 0 on success otherwise negative errno
>> + */
>> +static int cdns3_pci_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *id)
>> +{
>> +struct platform_device_info plat_info;
>> +struct cdns3_wrap *wrap;
>> +struct resource *res;
>> +int err;
>> +
>> +/*
>> + * for GADGET/HOST PCI (devfn) function number is 0,
>> + * for OTG PCI (devfn) function number is 1
>> + */
>> +if (!id || pdev->devfn != PCI_DEV_FN_HOST_DEVICE)
>> +return -EINVAL;
>> +
>> +err = pcim_enable_device(pdev);
>> +if (err) {
>> +dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
>> +return err;
>> +}
>> +
>> +pci_set_master(pdev);
>> +wrap = devm_kzalloc(&pdev->dev, sizeof(*wrap), GFP_KERNEL);
>> +if (!wrap) {
>> +dev_err(&pdev->dev, "Failed to allocate memory\n");
>> +return -ENOMEM;
>> +}
>> +
>> +/* function 0: host(BAR_0) + device(BAR_1) + otg(BAR_2)). */
>> +memset(wrap->dev_res, 0x00,
>> + sizeof(struct resource) * ARRAY_SIZE(wrap->dev_res));
>> +dev_dbg(&pdev->dev, "Initialize Device resources\n");
>> +res = wrap->dev_res;
>> +
>> +res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
>> +res[RES_DEV_ID].end = pci_resource_end(pdev, PCI_BAR_DEV);
>> +res[RES_DEV_ID].name = "cdns3-dev-regs";
>> +res[RES_DEV_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(&pdev->dev, "USBSS-DEV physical base addr: %pa\n",
>> +&res[RES_DEV_ID].start);
>> +
>> +res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
>> +res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
>> +res[RES_HOST_ID].name = "cdns3-xhci-regs";
>> +res[RES_HOST_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(&pdev->dev, "USBSS-XHCI physical base addr: %pa\n",
>> +&res[RES_HOST_ID].start);
>> +
>> +res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
>> +res[RES_DRD_ID].end = pci_resource_end(pdev, PCI_BAR_OTG);
>> +res[RES_DRD_ID].name = "cdns3-otg";
>> +res[RES_DRD_ID].flags = IORESOURCE_MEM;
>> +dev_dbg(&pdev->dev, "USBSS-DRD physical base addr: %pa\n",
>> +&res[RES_DRD_ID].start);
>> +
>> +/* Interrupt common for both device and XHCI */
>> +wrap->dev_res[RES_IRQ_ID].start = pdev->irq;
>> +wrap->dev_res[RES_IRQ_ID].name = "cdns3-irq";
>> +wrap->dev_res[RES_IRQ_ID].flags = IORESOURCE_IRQ;
>> +
>> +/* set up platform device info */
>> +memset(&plat_info, 0, sizeof(plat_info));
>> +plat_info.parent = &pdev->dev;
>> +plat_info.fwnode = pdev->dev.fwnode;
>> +plat_info.name = PLAT_DRIVER_NAME;
>> +plat_info.id = pdev->devfn;
>> +plat_info.res = wrap->dev_res;
>> +plat_info.num_res = ARRAY_SIZE(wrap->dev_res);
>> +plat_info.dma_mask = pdev->dma_mask;
>> +
>> +/* register platform device */
>> +wrap->plat_dev = platform_device_register_full(&plat_info);
>> +if (IS_ERR(wrap->plat_dev)) {
>> +err = PTR_ERR(wrap->plat_dev);
>> +return err;
>> +}
>return PTR_ERR(wrap->plat_dev);
Ok.
>
>> +
>> +pci_set_drvdata(pdev, wrap);
>> +
>> +return err;
>> +}
>> +
>> +void cdns3_pci_remove(struct pci_dev *pdev)
>> +{
>> +struct cdns3_wrap *wrap = (struct cdns3_wrap *)pci_get_drvdata(pdev);
>> +
>> +platform_device_unregister(wrap->plat_dev);
>> +}
>> +
>> +static const struct pci_device_id cdns3_pc
RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi
I can't recreate these issue on my environment. I use gcc (Ubuntu
7.3.0-27ubuntu1~18.04) 7.3.0. Maybe there are some differenct between these
two compilers.
Do you have any idea how I should change this fragment of code:
sprintf(str, "%02x %02x %02x %02x %02x %02x %02x %02x",
bRequestType, bRequest,
cpu_to_le16(wValue) & 0xff,
cpu_to_le16(wValue) >> 8,
cpu_to_le16(wIndex) & 0xff,
cpu_to_le16(wIndex) >> 8,
cpu_to_le16(wLength) & 0xff,
cpu_to_le16(wLength) >> 8);
to remove "restricted __le16 degrades to integer" warnings ?
Maybe I should cast all to u8. Then this code will look like:
sprintf(str, "%02x %02x %02x %02x %02x %02x %02x %02x",
bRequestType, bRequest,
(u8)(cpu_to_le16(wValue) & 0xff),
(u8)(cpu_to_le16(wValue) >> 8),
(u8)(cpu_to_le16(wIndex) & 0xff),
(u8)(cpu_to_le16(wIndex) >> 8),
(u8)(cpu_to_le16(wLength) & 0xff),
(u8)(cpu_to_le16(wLength) >> 8));
Should it Fix these warnings ?
Cheers,
Pawel
>Thank you for the patch! Perhaps something to improve:
>
>
>
>[auto build test WARNING on usb/usb-testing]
>
>[also build test WARNING on v4.20-rc7 next-20181221]
>
>[if your patch is applied to the wrong git tree, please drop us a note to help
>improve the system]
>
>
>
>url:
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_0day-2Dci_linux_commits_Pawel-2DLaszczak_Introduced-
>2Dnew-2DCadence-2DUSBSS-2DDRD-2DDriver_20181223-2D231813&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-
>_haXqY&r=e1OgxfvkL0qo9XO6fX1gscva-w03uSYC1nIyxl89-rI&m=JS8MUBEUPN46-me57xUY-
>7hoBbSrlgd2SCB9ahNjK4s&s=bhpHqRyEtMdMbWdGoBqQ9Pz9wq7pRA7-OohrGik3BpM&e=
>
>base: https://urldefense.proofpoint.com/v2/url?u=https-
>3A__git.kernel.org_pub_scm_linux_kernel_git_gregkh_usb.git&d=DwIBAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-
>_haXqY&r=e1OgxfvkL0qo9XO6fX1gscva-w03uSYC1nIyxl89-rI&m=JS8MUBEUPN46-me57xUY-
>7hoBbSrlgd2SCB9ahNjK4s&s=Vf__lGpV27zdf1egowm9p8YBJjz9aMmgbi8nW_Z_DLk&e=
>usb-testing
>
>config: x86_64-allmodconfig (attached as .config)
>
>compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
>
>reproduce:
>
># save the attached .config to linux build tree
>
>make ARCH=x86_64
>
>
>
>All warnings (new ones prefixed by >>):
>
>
>
>>> drivers/usb/common/debug.c:253:25: warning: restricted __le16 degrades to
>>> integer
>
> drivers/usb/common/debug.c:254:25: warning: restricted __le16 degrades to
> integer
>
> drivers/usb/common/debug.c:255:25: warning: restricted __le16 degrades to
> integer
>
> drivers/usb/common/debug.c:256:25: warning: restricted __le16 degrades to
> integer
>
> drivers/usb/common/debug.c:257:25: warning: restricted __le16 degrades to
> integer
>
> drivers/usb/common/debug.c:258:25: warning: restricted __le16 degrades to
> integer
>
>
>
>vim +253 drivers/usb/common/debug.c
>
>
>
>cefb8b21 Pawel Laszczak 2018-12-23 205
>
>cefb8b21 Pawel Laszczak 2018-12-23 206 /**
>
>cefb8b21 Pawel Laszczak 2018-12-23 207 * usb_decode_ctrl - returns a string
>representation of ctrl request
>
>cefb8b21 Pawel Laszczak 2018-12-23 208 */
>
>cefb8b21 Pawel Laszczak 2018-12-23 209 const char *usb_decode_ctrl(char
>*str, __u8 bRequestType, __u8 bRequest,
>
>cefb8b21 Pawel Laszczak 2018-12-23 210__u16
>wValue, __u16 wIndex, __u16 wLength)
>
>cefb8b21 Pawel Laszczak 2018-12-23 211 {
>
>cefb8b21 Pawel Laszczak 2018-12-23 212switch (bRequest) {
>
>cefb8b21 Pawel Laszczak 2018-12-23 213case USB_REQ_GET_STATUS:
>
>cefb8b21 Pawel Laszczak 2018-12-23 214
>usb_decode_get_status(bRequestType, wIndex, wLength, str);
>
>cefb8b21 Pawel Laszczak 2018-12-23 215break;
>
>cefb8b21 Pawel Laszczak 2018-12-23 216case USB_REQ_CLEAR_FEATURE:
>
>cefb8b21 Pawel Laszczak 2018-12-23 217case USB_REQ_SET_FEATURE:
>
>cefb8b21 Pawel Laszczak 2018-12-23 218
>usb_decode_set_clear_feature(bRequestType, bRequest, wValue,
>
>cefb8b21 Pawel Laszczak 2018-12-23 219
> wIndex, str);
>
>cefb8b21 Pawel Laszczak 2018-12-23 220break;
>
>cefb8b21 Pawel Laszczak 2018-12-23 221case USB_REQ_SET_ADDRESS:
>
>cefb8b21 Pawel Laszczak 2018-12-23 222
>usb_decode_set_address(wValue, str);
>
>cefb8b21 Pawel Laszczak 2018-12-23 223break;
>
>cefb8b21 Pawel Laszczak 2018-12-23 224case USB_REQ_GET_DESCRIPTOR:
>
>cefb8b21 Pawel Laszczak 2018-12-23 225case USB_REQ_SET_DESCRIPTOR:
>
>cefb8b21 Pawel Laszczak 2018-12-23 226
>usb_decode_get_set_descriptor(bRequestType, bRequest, wValue,
>
>cefb8b21 Pawel Laszczak 2018-12-23
RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi Peter,
>
>> >
>> >@@ -299,6 +306,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
>> >cdns->version = CDNS3_CONTROLLER_V0;
>> >cdns->otg_v1_regs = NULL;
>> >cdns->otg_regs = regs;
>> >+ writel(0x1, &cdns->otg_v0_regs->simulate);
>> >dev_info(cdns->dev, "DRD version v0 (%08x)\n",
>> > readl(&cdns->otg_v0_regs->version));
>> >} else {
>>
>> I have confirmation from HW team that time that driver should wait after de-
>> selecting mode is 2-3ms for simulate mode. It's time when FSM is in
>> DRD_H_WAIT_VBUS_FAIL.
>> Driver cannot re-enable the host/device mode before this time has elapsed.
>>
>> 3 ms is the maximum time. Additionally, you can confirm the current FSM
>> state by
>> reading the host_otg_state (bit 5:3) or dev_otg_state (2:0) from OTGSTATE
>> register.
>>
>> If bit 0 in simulate register is cleared the time is exactly 1s.
>>
>
>Thanks, Pawel.
>
>Would you please add below changes in your next revision?
>- Set bit 0 in simulate register
But it's used only for simulation environments to speed up simulation.
On real platforms this bit should be cleared. I'm not sure if I can
add some code related to simulation environment to driver.
If yes then I must introduce the way, that allow to recognize this two modes.
I could add module parameter or add additional config in Kconfig file.
>- timeout logic for waiting host_otg_state or dev_otg_state at OTGSTATE
>when switch to host or device.
I will add such code.
Pawel
Re: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
hi,
On Sun, 2018-12-23 at 15:13 +, Pawel Laszczak wrote:
> This patch introduce new Cadence USBSS DRD driver
> to linux kernel.
<...>
> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c
> b/drivers/usb/cdns3/cdns3-pci-wrap.c
> new file mode 100644
> index ..e93179c45ece
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS PCI Glue driver
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct cdns3_wrap {
> + struct platform_device *plat_dev;
> + struct pci_dev *hg_dev;
> + struct resource dev_res[4];
> +};
> +
> +struct cdns3_wrap wrap;
> +
> +#define RES_IRQ_ID 0
> +#define RES_HOST_ID 1
> +#define RES_DEV_ID 2
> +#define RES_DRD_ID 3
> +
> +#define PCI_BAR_HOST 0
> +#define PCI_BAR_DEV 2
> +#define PCI_BAR_OTG 4
> +
> +#define PCI_DEV_FN_HOST_DEVICE 0
> +#define PCI_DEV_FN_OTG 1
> +
> +#define PCI_DRIVER_NAME "cdns3-pci-usbss"
> +#define PLAT_DRIVER_NAME "cdns-usb3"
> +
> +#define CDNS_VENDOR_ID 0x17cd
> +#define CDNS_DEVICE_ID 0x0100
> +
> +/**
> + * cdns3_pci_probe - Probe function for Cadence USB wrapper driver
> + * @pdev: platform device object
> + * @id: pci device id
> + *
> + * Returns 0 on success otherwise negative errno
> + */
> +static int cdns3_pci_probe(struct pci_dev *pdev,
> +const struct pci_device_id *id)
> +{
> + struct platform_device_info plat_info;
> + struct cdns3_wrap *wrap;
> + struct resource *res;
> + int err;
> +
> + /*
> + * for GADGET/HOST PCI (devfn) function number is 0,
> + * for OTG PCI (devfn) function number is 1
> + */
> + if (!id || pdev->devfn != PCI_DEV_FN_HOST_DEVICE)
> + return -EINVAL;
> +
> + err = pcim_enable_device(pdev);
> + if (err) {
> + dev_err(&pdev->dev, "Enabling PCI device has failed %d\n", err);
> + return err;
> + }
> +
> + pci_set_master(pdev);
> + wrap = devm_kzalloc(&pdev->dev, sizeof(*wrap), GFP_KERNEL);
> + if (!wrap) {
> + dev_err(&pdev->dev, "Failed to allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + /* function 0: host(BAR_0) + device(BAR_1) + otg(BAR_2)). */
> + memset(wrap->dev_res, 0x00,
> +sizeof(struct resource) * ARRAY_SIZE(wrap->dev_res));
> + dev_dbg(&pdev->dev, "Initialize Device resources\n");
> + res = wrap->dev_res;
> +
> + res[RES_DEV_ID].start = pci_resource_start(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].end = pci_resource_end(pdev, PCI_BAR_DEV);
> + res[RES_DEV_ID].name = "cdns3-dev-regs";
> + res[RES_DEV_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "USBSS-DEV physical base addr: %pa\n",
> + &res[RES_DEV_ID].start);
> +
> + res[RES_HOST_ID].start = pci_resource_start(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].end = pci_resource_end(pdev, PCI_BAR_HOST);
> + res[RES_HOST_ID].name = "cdns3-xhci-regs";
> + res[RES_HOST_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "USBSS-XHCI physical base addr: %pa\n",
> + &res[RES_HOST_ID].start);
> +
> + res[RES_DRD_ID].start = pci_resource_start(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].end = pci_resource_end(pdev, PCI_BAR_OTG);
> + res[RES_DRD_ID].name = "cdns3-otg";
> + res[RES_DRD_ID].flags = IORESOURCE_MEM;
> + dev_dbg(&pdev->dev, "USBSS-DRD physical base addr: %pa\n",
> + &res[RES_DRD_ID].start);
> +
> + /* Interrupt common for both device and XHCI */
> + wrap->dev_res[RES_IRQ_ID].start = pdev->irq;
> + wrap->dev_res[RES_IRQ_ID].name = "cdns3-irq";
> + wrap->dev_res[RES_IRQ_ID].flags = IORESOURCE_IRQ;
> +
> + /* set up platform device info */
> + memset(&plat_info, 0, sizeof(plat_info));
> + plat_info.parent = &pdev->dev;
> + plat_info.fwnode = pdev->dev.fwnode;
> + plat_info.name = PLAT_DRIVER_NAME;
> + plat_info.id = pdev->devfn;
> + plat_info.res = wrap->dev_res;
> + plat_info.num_res = ARRAY_SIZE(wrap->dev_res);
> + plat_info.dma_mask = pdev->dma_mask;
> +
> + /* register platform device */
> + wrap->plat_dev = platform_device_register_full(&plat_info);
> + if (IS_ERR(wrap->plat_dev)) {
> + err = PTR_ERR(wrap->plat_dev);
> + return err;
> + }
return PTR_ERR(wrap->plat_dev);
> +
> + pci_set_drvdata(pdev, wrap);
> +
> + return err;
> +}
> +
> +void cdns3_pci_remove(struct pci_dev *pdev)
> +{
> + struct cdns3_wrap *wrap = (struct cdns3_wrap *)pci_get_drvdata(pdev);
> +
> + platform_device_unregister(wrap->plat_dev);
> +}
> +
> +static const struct pci_device_id cdns3_pci_ids[] = {
> + { PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
> + { 0, }
>
RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
> >
> >@@ -299,6 +306,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
> >cdns->version = CDNS3_CONTROLLER_V0;
> >cdns->otg_v1_regs = NULL;
> >cdns->otg_regs = regs;
> >+ writel(0x1, &cdns->otg_v0_regs->simulate);
> >dev_info(cdns->dev, "DRD version v0 (%08x)\n",
> > readl(&cdns->otg_v0_regs->version));
> >} else {
>
> I have confirmation from HW team that time that driver should wait after de-
> selecting mode is 2-3ms for simulate mode. It's time when FSM is in
> DRD_H_WAIT_VBUS_FAIL.
> Driver cannot re-enable the host/device mode before this time has elapsed.
>
> 3 ms is the maximum time. Additionally, you can confirm the current FSM state
> by
> reading the host_otg_state (bit 5:3) or dev_otg_state (2:0) from OTGSTATE
> register.
>
> If bit 0 in simulate register is cleared the time is exactly 1s.
>
Thanks, Pawel.
Would you please add below changes in your next revision?
- Set bit 0 in simulate register
- timeout logic for waiting host_otg_state or dev_otg_state at OTGSTATE
when switch to host or device.
Peter
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 ..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 ..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 > + *
>> + */
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#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);
RE: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi,
>> > +
>> > + 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.
>>
>
>Pawel, since OTG compliance (Known as HNP/SRP) has not been used
>widely, Linux kernel does not
>maintain it from some time ago (maybe 1-2 years). In software design,
>we do not need to consider it from
>hardware point, eg, kinds of OTG timer. For dual-role switch on the
>fly, through /sys is enough.
>
>Through the debug, we find it needs to wait 1s after setting de-select
>the host or gadget before request
>XHCI at otg_regs->cmd, and enable fast simulate can reduce delay to
>2-3ms. Would you please help
>to check with your hardware team this behavior. With below changes, I
>can get OTGSTS_XHCI_READY at otgsts.
>
>@@ -141,6 +143,7 @@ static int cdns3_drd_switch_host(struct cdns3 *cdns, int
>on)
>writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> &cdns->otg_regs->cmd);
>+ usleep_range(3000, 4000);
>}
>
>return 0;
>@@ -178,6 +181,7 @@ static int cdns3_drd_switch_gadget(struct cdns3
>*cdns, int on)
>writel(OTGCMD_HOST_BUS_DROP | OTGCMD_DEV_BUS_DROP |
> OTGCMD_DEV_POWER_OFF | OTGCMD_HOST_POWER_OFF,
> &cdns->otg_regs->cmd);
>+ usleep_range(3000, 4000);
>}
>
>@@ -299,6 +306,7 @@ int cdns3_drd_init(struct cdns3 *cdns)
>cdns->version = CDNS3_CONTROLLER_V0;
>cdns->otg_v1_regs = NULL;
>cdns->otg_regs = regs;
>+ writel(0x1, &cdns->otg_v0_regs->simulate);
>dev_info(cdns->dev, "DRD version v0 (%08x)\n",
> readl(&cdns->otg_v0_regs->version));
>} else {
I have confirmation from HW team that time that driver should wait after
de-selecting mode
is 2-3ms for simulate mode. It's time when FSM is in DRD_H_WAIT_VBUS_FAIL.
Driver cannot re-enable the host/device mode before this time has elapsed.
3 ms is the maximum time. Additionally, you can confirm the current FSM state
by reading the
host_otg_state (bit 5:3) or dev_otg_state (2:0) from OTGSTATE register.
If bit 0 in simulate register is cleared the time is exactly 1s.
Cheers,
Pawel
Re: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi Pawel,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.20-rc7 next-20181221]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Pawel-Laszczak/Introduced-new-Cadence-USBSS-DRD-Driver/20181223-231813
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
usb-testing
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
>> drivers/usb/common/debug.c:253:25: warning: restricted __le16 degrades to
>> integer
drivers/usb/common/debug.c:254:25: warning: restricted __le16 degrades to
integer
drivers/usb/common/debug.c:255:25: warning: restricted __le16 degrades to
integer
drivers/usb/common/debug.c:256:25: warning: restricted __le16 degrades to
integer
drivers/usb/common/debug.c:257:25: warning: restricted __le16 degrades to
integer
drivers/usb/common/debug.c:258:25: warning: restricted __le16 degrades to
integer
vim +253 drivers/usb/common/debug.c
cefb8b21 Pawel Laszczak 2018-12-23 205
cefb8b21 Pawel Laszczak 2018-12-23 206 /**
cefb8b21 Pawel Laszczak 2018-12-23 207 * usb_decode_ctrl - returns a string
representation of ctrl request
cefb8b21 Pawel Laszczak 2018-12-23 208 */
cefb8b21 Pawel Laszczak 2018-12-23 209 const char *usb_decode_ctrl(char *str,
__u8 bRequestType, __u8 bRequest,
cefb8b21 Pawel Laszczak 2018-12-23 210 __u16
wValue, __u16 wIndex, __u16 wLength)
cefb8b21 Pawel Laszczak 2018-12-23 211 {
cefb8b21 Pawel Laszczak 2018-12-23 212 switch (bRequest) {
cefb8b21 Pawel Laszczak 2018-12-23 213 case USB_REQ_GET_STATUS:
cefb8b21 Pawel Laszczak 2018-12-23 214
usb_decode_get_status(bRequestType, wIndex, wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 215 break;
cefb8b21 Pawel Laszczak 2018-12-23 216 case USB_REQ_CLEAR_FEATURE:
cefb8b21 Pawel Laszczak 2018-12-23 217 case USB_REQ_SET_FEATURE:
cefb8b21 Pawel Laszczak 2018-12-23 218
usb_decode_set_clear_feature(bRequestType, bRequest, wValue,
cefb8b21 Pawel Laszczak 2018-12-23 219
wIndex, str);
cefb8b21 Pawel Laszczak 2018-12-23 220 break;
cefb8b21 Pawel Laszczak 2018-12-23 221 case USB_REQ_SET_ADDRESS:
cefb8b21 Pawel Laszczak 2018-12-23 222
usb_decode_set_address(wValue, str);
cefb8b21 Pawel Laszczak 2018-12-23 223 break;
cefb8b21 Pawel Laszczak 2018-12-23 224 case USB_REQ_GET_DESCRIPTOR:
cefb8b21 Pawel Laszczak 2018-12-23 225 case USB_REQ_SET_DESCRIPTOR:
cefb8b21 Pawel Laszczak 2018-12-23 226
usb_decode_get_set_descriptor(bRequestType, bRequest, wValue,
cefb8b21 Pawel Laszczak 2018-12-23 227
wIndex, wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 228 break;
cefb8b21 Pawel Laszczak 2018-12-23 229 case USB_REQ_GET_CONFIGURATION:
cefb8b21 Pawel Laszczak 2018-12-23 230
usb_decode_get_configuration(wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 231 break;
cefb8b21 Pawel Laszczak 2018-12-23 232 case USB_REQ_SET_CONFIGURATION:
cefb8b21 Pawel Laszczak 2018-12-23 233
usb_decode_set_configuration(wValue, str);
cefb8b21 Pawel Laszczak 2018-12-23 234 break;
cefb8b21 Pawel Laszczak 2018-12-23 235 case USB_REQ_GET_INTERFACE:
cefb8b21 Pawel Laszczak 2018-12-23 236
usb_decode_get_intf(wIndex, wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 237 break;
cefb8b21 Pawel Laszczak 2018-12-23 238 case USB_REQ_SET_INTERFACE:
cefb8b21 Pawel Laszczak 2018-12-23 239
usb_decode_set_intf(wValue, wIndex, str);
cefb8b21 Pawel Laszczak 2018-12-23 240 break;
cefb8b21 Pawel Laszczak 2018-12-23 241 case USB_REQ_SYNCH_FRAME:
cefb8b21 Pawel Laszczak 2018-12-23 242
usb_decode_synch_frame(wIndex, wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 243 break;
cefb8b21 Pawel Laszczak 2018-12-23 244 case USB_REQ_SET_SEL:
cefb8b21 Pawel Laszczak 2018-12-23 245
usb_decode_set_sel(wLength, str);
cefb8b21 Pawel Laszczak 2018-12-23 246 break;
cefb8b21 Pawel Laszczak 2018-12-23 247 case USB_REQ_SET_ISOCH_DELAY:
cefb8b21 Pawel Laszczak 2018-12-23 248
usb_decode_set_isoch_delay(wValue, str);
cefb8b21 Pawel Laszczak 2018-12-23 249 break;
cefb8b21 Pawel Laszczak 2018-12-23 250 default:
cefb8b21 Pawel Laszczak 2018-12-23 251 sprintf(str, "%02x %02x
Re: [PATCH v2 5/5] usb:cdns3 Add Cadence USB3 DRD Driver
Hi, Here are a few Kconfig text corrections. On 12/23/18 7:13 AM, Pawel Laszczak wrote: > diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig > new file mode 100644 > index ..4adfd87811e8 > --- /dev/null > +++ b/drivers/usb/cdns3/Kconfig > @@ -0,0 +1,44 @@ > +config USB_CDNS3 > + tristate "Cadence USB3 Dual-Role Controller" > + depends on USB_SUPPORT && (USB || USB_GADGET) && HAS_DMA > + help > + Say Y here if your system has a cadence USB3 dual-role controller. > + It supports: dual-role switch, Host-only, and Peripheral-only. > + > + If you choose to build this driver is a dynamically linked as > + module, the module will be called cdns3.ko. > + > +if USB_CDNS3 > + > +config USB_CDNS3_GADGET > +bool "Cadence USB3 device controller" > +depends on USB_GADGET > +help > + Say Y here to enable device controller functionality of the > + cadence USBSS-DEV driver. > + > + This controller supports FF, HS and SS mode. It doesn't support > + LS and SSP mode mode. > + > +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 compliant > + standard XHCI driver. > + > +config USB_CDNS3_PCI_WRAP > + tristate "Cadence USB3 support on PCIe-based platforms" > + depends on USB_PCI && ACPI > + default USB_CDNS3 > + help > + If you're using the USBSS Core IP with a PCIe, please say > + 'Y' or 'M' here. > + > + If you choose to build this driver as module it will > + be dynamically linked and module will be called cdns3-pci.ko > + > +endif g'day. -- ~Randy
