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

[Pawel Laszczak <pawell@xxxxxxxxxxx>]

Hi Roger,

>
>On 21/07/2019 21:32, Pawel Laszczak wrote:
>> This patch introduce new Cadence USBSS DRD driver to Linux kernel.
>>
>> The Cadence USBSS DRD Controller is a highly configurable IP Core which
>> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
>> Host Only (XHCI)configurations.
>>
>> The current driver has been validated with FPGA platform. We have
>> support for PCIe bus, which is used on FPGA prototyping.
>>
>> The host side of USBSS-DRD controller is compliant with XHCI
>> specification, so it works with standard XHCI Linux driver.
>>
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>>  drivers/usb/Kconfig                |    2 +
>>  drivers/usb/Makefile               |    2 +
>>  drivers/usb/cdns3/Kconfig          |   46 +
>>  drivers/usb/cdns3/Makefile         |   17 +
>>  drivers/usb/cdns3/cdns3-pci-wrap.c |  203 +++
>>  drivers/usb/cdns3/core.c           |  554 +++++++
>>  drivers/usb/cdns3/core.h           |  109 ++
>>  drivers/usb/cdns3/debug.h          |  171 ++
>>  drivers/usb/cdns3/debugfs.c        |   87 ++
>>  drivers/usb/cdns3/drd.c            |  390 +++++
>>  drivers/usb/cdns3/drd.h            |  166 ++
>>  drivers/usb/cdns3/ep0.c            |  914 +++++++++++
>>  drivers/usb/cdns3/gadget-export.h  |   28 +
>>  drivers/usb/cdns3/gadget.c         | 2338 ++++++++++++++++++++++++++++
>>  drivers/usb/cdns3/gadget.h         | 1321 ++++++++++++++++
>>  drivers/usb/cdns3/host-export.h    |   28 +
>>  drivers/usb/cdns3/host.c           |   71 +
>>  drivers/usb/cdns3/trace.c          |   11 +
>>  drivers/usb/cdns3/trace.h          |  493 ++++++
>>  19 files changed, 6951 insertions(+)
>>  create mode 100644 drivers/usb/cdns3/Kconfig
>>  create mode 100644 drivers/usb/cdns3/Makefile
>>  create mode 100644 drivers/usb/cdns3/cdns3-pci-wrap.c
>>  create mode 100644 drivers/usb/cdns3/core.c
>>  create mode 100644 drivers/usb/cdns3/core.h
>>  create mode 100644 drivers/usb/cdns3/debug.h
>>  create mode 100644 drivers/usb/cdns3/debugfs.c
>>  create mode 100644 drivers/usb/cdns3/drd.c
>>  create mode 100644 drivers/usb/cdns3/drd.h
>>  create mode 100644 drivers/usb/cdns3/ep0.c
>>  create mode 100644 drivers/usb/cdns3/gadget-export.h
>>  create mode 100644 drivers/usb/cdns3/gadget.c
>>  create mode 100644 drivers/usb/cdns3/gadget.h
>>  create mode 100644 drivers/usb/cdns3/host-export.h
>>  create mode 100644 drivers/usb/cdns3/host.c
>>  create mode 100644 drivers/usb/cdns3/trace.c
>>  create mode 100644 drivers/usb/cdns3/trace.h
>>
>
><snip>
>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> new file mode 100644
>> index 000000000000..291f08be56fe
>> --- /dev/null
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -0,0 +1,2338 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBSS DRD Driver - gadget side.
>> + *
>> + * Copyright (C) 2018-2019 Cadence Design Systems.
>> + * Copyright (C) 2017-2018 NXP
>> + *
>> + * Authors: Pawel Jez <pjez@xxxxxxxxxxx>,
>> + *          Pawel Laszczak <pawell@xxxxxxxxxxx>
>> + *          Peter Chen <peter.chen@xxxxxxx>
>> + */
>> +
>
><snip>
>
>> +
>> +static void cdns3_gadget_config(struct cdns3_device *priv_dev)
>> +{
>> +	struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
>> +	u32 reg;
>> +
>> +	cdns3_ep0_config(priv_dev);
>> +
>> +	/* enable interrupts for endpoint 0 (in and out) */
>> +	writel(EP_IEN_EP_OUT0 | EP_IEN_EP_IN0, &regs->ep_ien);
>> +
>> +	/*
>> +	 * Driver needs to modify LFPS minimal U1 Exit time for DEV_VER_TI_V1
>> +	 * revision of controller.
>> +	 */
>> +	if (priv_dev->dev_ver == DEV_VER_TI_V1) {
>> +		reg = readl(&regs->dbg_link1);
>> +
>> +		reg &= ~DBG_LINK1_LFPS_MIN_GEN_U1_EXIT_MASK;
>> +		reg |= DBG_LINK1_LFPS_MIN_GEN_U1_EXIT(0x55) |
>> +		       DBG_LINK1_LFPS_MIN_GEN_U1_EXIT_SET;
>> +		writel(reg, &regs->dbg_link1);
>> +	}
>> +
>> +	/*
>> +	 * By default some platforms has set protected access to memory.
>> +	 * This cause problem with cache, so driver restore non-secure
>> +	 * access to memory.
>> +	 */
>> +	reg = readl(&regs->dma_axi_ctrl);
>
>Why read the reg at all if you are just overwriting it below?
>
>> +	reg = DMA_AXI_CTRL_MARPROT(DMA_AXI_CTRL_NON_SECURE) |
>> +	      DMA_AXI_CTRL_MAWPROT(DMA_AXI_CTRL_NON_SECURE);
>
>
>Otherwise you need to read modify only necessary bits and then write.

Yes, we also has found this bug. 
It's will be corrected in next version.

>i.e.
>	#define DMA_AXI_CTRL_MAPROT_MASK 0x3
>	reg &= ~(DMA_AXI_CTRL_MARPROT(DMA_AXI_CTRL_MAPROT_MASK) |
>		 DMA_AXI_CTRL_MARPROT(DMA_AXI_CTRL_MAPROT_MASK))
>	reg |= DMA_AXI_CTRL_MARPROT(DMA_AXI_CTRL_NON_SECURE) |
>	       DMA_AXI_CTRL_MAWPROT(DMA_AXI_CTRL_NON_SECURE);
>
>> +	writel(reg, &regs->dma_axi_ctrl);
>> +
>> +	/* enable generic interrupt*/
>> +	writel(USB_IEN_INIT, &regs->usb_ien);
>> +	writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, &regs->usb_conf);
>> +
>> +	cdns3_configure_dmult(priv_dev, NULL);
>> +
>> +	cdns3_gadget_pullup(&priv_dev->gadget, 1);
>> +}
>> +
>
><snip>
>

Cheers,
Pawell