RE: [PATCH v3 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL for usb gadget

[Leo Li]

> -Original Message-
> From: Tiago Brusamarello [mailto:tbr...@gmail.com]
> Sent: Thursday, May 17, 2018 4:29 PM
> To: Leo Li <leoyang...@nxp.com>
> Cc: linux-usb@vger.kernel.org; Nikhil Badola <nikhil.bad...@freescale.com>
> Subject: [PATCH v3 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL
> for usb gadget
> 
> From: Nikhil Badola <nikhil.bad...@freescale.com>
> 
> Introduce FSL_USB2_PHY_UTMI_DUAL in gadget driver for setting phy in
> SOCs with utmi dual phy
> 
> Signed-off-by: Nikhil Badola <nikhil.bad...@freescale.com>
> Tested-by: Tiago Brusamarello <tbr...@gmail.com>

Acked-by: Li Yang <leoyang...@nxp.com>

> ---
> 
> Changes since v2:
>  * Reformatted the patch so it can be applied in the main tree
> 
> Changes since v1:
>  * Removed Freescale internal information from commit message
> 
>  drivers/usb/gadget/udc/fsl_udc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
> b/drivers/usb/gadget/udc/fsl_udc_core.c
> index 56b517a38865..a3c092b4db20 100644
> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
> @@ -253,6 +253,7 @@ static int dr_controller_setup(struct fsl_udc *udc)
>   portctrl |= PORTSCX_PTW_16BIT;
>   /* fall through */
>   case FSL_USB2_PHY_UTMI:
> + case FSL_USB2_PHY_UTMI_DUAL:
>   if (udc->pdata->have_sysif_regs) {
>   if (udc->pdata->controller_ver) {
>   /* controller version 1.6 or above */
> --
> 2.12.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL for usb gadget

[Leo Li]

> -Original Message-
> From: Tiago Brusamarello [mailto:tbr...@gmail.com]
> Sent: Thursday, April 19, 2018 6:58 AM
> To: Leo Li <leoyang...@nxp.com>
> Cc: nikhil.bad...@freescale.com; linux-usb@vger.kernel.org
> Subject: [PATCH 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL for
> usb gadget
> 
> Introduce FSL_USB2_PHY_UTMI_DUAL in gadget driver for setting
> phy in SOCs with utmi dual phy
> 
> Signed-off-by: Nikhil Badola <mailto:nikhil.bad...@freescale.com>

[snip]
> Change-Id: I2c53b89d9916bd17b5be8b5d9e32454943172d55
> Reviewed-on:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.a
> m.freescale.net%3A8181%2F25528=02%7C01%7Cleoyang.li%40nxp.co
> m%7C7e18a2921b7b43abb8db08d5a5ecc059%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636597358611709638=U3moxDqSc%2F576XLDS
> qJLdfYhDJeccNLzF%2FWr%2BQv0bRU%3D=0
> Tested-by: Review Code-CDREVIEW <mailto:cdrev...@freescale.com>
> Reviewed-by: Suresh Gupta <mailto:suresh.gu...@freescale.com>
> Reviewed-by: Matthew Weigel <mailto:matthew.wei...@freescale.com>
[/snip]

The patch looks good.  But we probably should remove the above internal 
information as they are not needed for upstream.

> Tested-by: Tiago Brusamarello <mailto:tbr...@gmail.com>

Otherwise

Acked-by: Li Yang <leoyang...@nxp.com>

> ---
>  drivers/usb/gadget/udc/fsl_udc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
> b/drivers/usb/gadget/udc/fsl_udc_core.c
> index 56b517a..a3c092b 100644
> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
> @@ -253,6 +253,7 @@ static int dr_controller_setup(struct fsl_udc *udc)
>          portctrl |= PORTSCX_PTW_16BIT;
>          /* fall through */
>      case FSL_USB2_PHY_UTMI:
> +    case FSL_USB2_PHY_UTMI_DUAL:
>          if (udc->pdata->have_sysif_regs) {
>              if (udc->pdata->controller_ver) {
>                  /* controller version 1.6 or above */
> --
> 2.7.4

RE: [PATCH] usb: gadget: fsl_udc_core: fix ep valid checks

[Leo Li]

> -Original Message-
> From: Stefan Agner [mailto:ste...@agner.ch]
> Sent: Monday, February 12, 2018 7:15 AM
> To: Leo Li <leoyang...@nxp.com>; ba...@kernel.org
> Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; Stefan Agner
> <ste...@agner.ch>
> Subject: [PATCH] usb: gadget: fsl_udc_core: fix ep valid checks
> 
> Clang reports the following warning:
>   drivers/usb/gadget/udc/fsl_udc_core.c:1312:10: warning: address of array
>   'ep->name' will always evaluate to 'true' [-Wpointer-bool-conversion]
> if (ep->name)
> ~~  ^~~~
> 
> It seems that the authors intention was to check if the ep has been
> configured through struct_ep_setup. Check whether struct usb_ep name
> pointer has been set instead.
> 
> Signed-off-by: Stefan Agner <ste...@agner.ch>

Acked-by: Li Yang <leoyang...@nxp.com>

> ---
>  drivers/usb/gadget/udc/fsl_udc_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
> b/drivers/usb/gadget/udc/fsl_udc_core.c
> index e5b4ee96c4bf..56b517a38865 100644
> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
> @@ -1305,7 +1305,7 @@ static void udc_reset_ep_queue(struct fsl_udc
> *udc, u8 pipe)  {
>   struct fsl_ep *ep = get_ep_by_pipe(udc, pipe);
> 
> - if (ep->name)
> + if (ep->ep.name)
>   nuke(ep, -ESHUTDOWN);
>  }
> 
> @@ -1693,7 +1693,7 @@ static void dtd_complete_irq(struct fsl_udc *udc)
>   curr_ep = get_ep_by_pipe(udc, i);
> 
>   /* If the ep is configured */
> - if (curr_ep->name == NULL) {
> + if (!curr_ep->ep.name) {
>   WARNING("Invalid EP?");
>   continue;
>   }
> --
> 2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-devel] [PATCH 3/3] core-arm: add a member about dma_map_ops in dev_archdata struct

[Leo Li]

> -Original Message-
> From: linux-devel-boun...@gforge.freescale.net [mailto:linux-devel-
> boun...@gforge.freescale.net] On Behalf Of yinbo@nxp.com
> Sent: Friday, August 11, 2017 5:01 AM
> To: linux-de...@gforge.freescale.net; Yinbo Zhu ; Rob
> Herring ; Mark Rutland ;
> Russell King ; Felipe Balbi 
> Cc: open list ; Laurent Pinchart
> ; Catalin Marinas
> ; open list ; open list
> ; Doug Ledford ;
> Stefano Stabellini ; Greg Kroah-Hartman
> ; Bart Van Assche
> ; moderated list  ker...@lists.infradead.org>
> Subject: [linux-devel] [PATCH 3/3] core-arm: add a member about
> dma_map_ops in dev_archdata struct
> 
> From: "yinbo.zhu" 

The dma_ops was just moved from struct dev_archdata to struct device with 
commit 5657933dbb6e25feaf5d8df8c88f96cdade693a3.  Don't duplicate the dma_ops 
in both places if you don't have a good reason.

> 
> Signed-off-by: yinbo.zhu 
> ---
>  arch/arm/include/asm/device.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
> index 220ba20..c93d3df 100644
> --- a/arch/arm/include/asm/device.h
> +++ b/arch/arm/include/asm/device.h
> @@ -7,6 +7,7 @@
>  #define ASMARM_DEVICE_H
> 
>  struct dev_archdata {
> +const struct dma_map_ops*dma_ops;
>  #ifdef CONFIG_DMABOUNCE
>   struct dmabounce_device_info *dmabounce;  #endif
> --
> 2.1.0.27.g96db324
> 
> ___
> linux-devel mailing list
> linux-de...@gforge.freescale.net
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgforge.
> freescale.net%2Fmailman%2Flistinfo%2Flinux-
> devel=01%7C01%7Cleoyang.li%40nxp.com%7Cc83542988dac448bd8b908
> d4e0a230af%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0=1pbSjxG781
> yjcCF%2BuXbRCkV6ytbfTTgQkHNWmm0e9nk%3D=0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-devel] [PATCH 1/3] dts: usb3: Add configure-gfladj property to USB3 nod

[Leo Li]

> -Original Message-
> From: linux-devel-boun...@gforge.freescale.net [mailto:linux-devel-
> boun...@gforge.freescale.net] On Behalf Of yinbo@nxp.com
> Sent: Friday, August 11, 2017 4:30 AM
> To: linux-de...@gforge.freescale.net; Yinbo Zhu ; Rob
> Herring ; Mark Rutland ;
> Russell King ; Felipe Balbi 
> Cc: open list ; Laurent Pinchart
> ; Catalin Marinas
> ; open list ; open list
> ; Doug Ledford ;
> Stefano Stabellini ; Greg Kroah-Hartman
> ; Bart Van Assche
> ; moderated list  ker...@lists.infradead.org>
> Subject: [linux-devel] [PATCH 1/3] dts: usb3: Add configure-gfladj property to
> USB3 nod
> 
> From: "yinbo.zhu" 
> 

There is no device tree binding for this property.  You need to add the binding 
first before you add it to the dts.  And normally you need to describe why the 
change is needed in the commit message.

> Signed-off-by: yinbo.zhu 
> ---
>  arch/arm/boot/dts/ls1021a.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index ffbf3cf..f525297 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -731,6 +731,8 @@
>   interrupts = ;
>   dr_mode = "host";
>   snps,quirk-frame-length-adjustment = <0x20>;
> + configure-gfladj;
> + dma-coherent;

This change is not aligned with your patch title.

>   snps,dis_rxdet_inp3_quirk;
>   };
> 
> --
> 2.1.0.27.g96db324
> 
> ___
> linux-devel mailing list
> linux-de...@gforge.freescale.net
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgforge.
> freescale.net%2Fmailman%2Flistinfo%2Flinux-
> devel=01%7C01%7Cleoyang.li%40nxp.com%7C015b054c76ad48399ea408
> d4e09de91d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0=7ioU%2Bb
> QUP4X%2F8%2BFpcQZICVDHugioStScbrHc0XOqaQs%3D=0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-devel] [PATCH 2/3] usb: dwc3 : Add support for USB snooping

[Leo Li]

> -Original Message-
> From: linux-devel-boun...@gforge.freescale.net [mailto:linux-devel-
> boun...@gforge.freescale.net] On Behalf Of yinbo@nxp.com
> Sent: Friday, August 11, 2017 5:00 AM
> To: linux-de...@gforge.freescale.net; Yinbo Zhu ; Rob
> Herring ; Mark Rutland ;
> Russell King ; Felipe Balbi 
> Cc: open list ; Laurent Pinchart
> ; Catalin Marinas
> ; open list ; open list
> ; Doug Ledford ;
> Stefano Stabellini ; Greg Kroah-Hartman
> ; Bart Van Assche
> ; moderated list  ker...@lists.infradead.org>
> Subject: [linux-devel] [PATCH 2/3] usb: dwc3 : Add support for USB snooping
> 
> From: Rajesh Bhagat 
> 
> Add support for USB3 snooping by asserting bits in register DWC3_GSBUSCFG0
> for data and descriptor

The description doesn't fully cover the change you made below, for example 
allocating multiple event buffers.  Please explain why you made such change.

> 
> Signed-off-by: Nikhil Badola 
> Signed-off-by: Rajesh Bhagat 
> Signed-off-by: yinbo.zhu 
> ---
>  drivers/usb/dwc3/core.c | 71 ---
> --
>  drivers/usb/dwc3/core.h |  3 +++
>  drivers/usb/dwc3/host.c |  8 +-
>  3 files changed, 63 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> 02a534a..b51b0d8 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -90,6 +90,7 @@ static int dwc3_get_dr_mode(struct dwc3 *dwc)
> 
>   if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
>   mode = USB_DR_MODE_HOST;
> +

Why are you adding a new line here?

>   else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
>   mode = USB_DR_MODE_PERIPHERAL;
>   }
> @@ -305,14 +306,27 @@ static void dwc3_free_event_buffers(struct dwc3
> *dwc)
>   */
>  static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length)  {
> - struct dwc3_event_buffer *evt;
> + int num;
> + int i;
> +
> + num = DWC3_NUM_INT(dwc->hwparams.hwparams1);
> + dwc->num_event_buffers = num;
> +
> + dwc->ev_buffs = devm_kzalloc(dwc->dev, sizeof(*dwc->ev_buffs) * num,
> + GFP_KERNEL);
> + if (!dwc->ev_buffs)
> + return -ENOMEM;
> 
> - evt = dwc3_alloc_one_event_buffer(dwc, length);
> - if (IS_ERR(evt)) {
> - dev_err(dwc->dev, "can't allocate event buffer\n");
> - return PTR_ERR(evt);
> + for (i = 0; i < num; i++) {
> + struct dwc3_event_buffer*evt;
> +
> + evt = dwc3_alloc_one_event_buffer(dwc, length);
> + if (IS_ERR(evt)) {
> + dev_err(dwc->dev, "can't allocate event buffer\n");
> + return PTR_ERR(evt);
> + }
> + dwc->ev_buffs[i] = evt;
>   }
> - dwc->ev_buf = evt;
> 
>   return 0;
>  }
> @@ -325,17 +339,25 @@ static int dwc3_alloc_event_buffers(struct dwc3
> *dwc, unsigned length)
>   */
>  static int dwc3_event_buffers_setup(struct dwc3 *dwc)  {
> - struct dwc3_event_buffer*evt;
> -
> - evt = dwc->ev_buf;
> - evt->lpos = 0;
> - dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0),
> - lower_32_bits(evt->dma));
> - dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(0),
> - upper_32_bits(evt->dma));
> - dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0),
> - DWC3_GEVNTSIZ_SIZE(evt->length));
> - dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), 0);
> + struct dwc3_event_buffer*evt;
> + int n;
> +
> + for (n = 0; n < dwc->num_event_buffers; n++) {
> + evt = dwc->ev_buffs[n];
> + dev_dbg(dwc->dev, "Event buf %p dma %08llx length %d\n",
> + evt->buf, (unsigned long long) evt->dma,
> + evt->length);
> +
> + evt->lpos = 0;
> +
> + dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(n),
> + lower_32_bits(evt->dma));
> + dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(n),
> + upper_32_bits(evt->dma));
> + dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(n),
> + DWC3_GEVNTSIZ_SIZE(evt->length));
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(n), 0);
> + }
> 
>   return 0;
>  }
> @@ -1181,6 +1203,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
> static int dwc3_probe(struct platform_device *pdev)  {
>   struct 

RE: [PATCH] usb: gadget: fsl_qe_udc: constify qe_ep0_desc

[Leo Li]

> -Original Message-
> From: Julia Lawall [mailto:julia.law...@lip6.fr]
> Sent: Wednesday, August 02, 2017 10:29 AM
> To: Leo Li <leoyang...@nxp.com>
> Cc: kernel-janit...@vger.kernel.org; Felipe Balbi <ba...@kernel.org>; Greg
> Kroah-Hartman <gre...@linuxfoundation.org>; linux-usb@vger.kernel.org;
> linuxppc-...@lists.ozlabs.org; linux-ker...@vger.kernel.org; Bhumika Goyal
> <bhumi...@gmail.com>
> Subject: [PATCH] usb: gadget: fsl_qe_udc: constify qe_ep0_desc
> 
> qe_ep0_desc is only passed as the second argument to qe_ep_init, which is
> const, so qe_ep0_desc can be const too.
> 
> Done with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>

Acked-by: Li Yang <leoyang...@nxp.com>

> 
> ---
> I got a lot of warnings when compiling this file, but none seemed to be 
> related
> to the change.
> 
>  drivers/usb/gadget/udc/fsl_qe_udc.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c
> b/drivers/usb/gadget/udc/fsl_qe_udc.c
> index 303328ce..a3e72d6 100644
> --- a/drivers/usb/gadget/udc/fsl_qe_udc.c
> +++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
> @@ -62,7 +62,7 @@
>   "ep3",
>  };
> 
> -static struct usb_endpoint_descriptor qe_ep0_desc = {
> +static const struct usb_endpoint_descriptor qe_ep0_desc = {
>   .bLength =  USB_DT_ENDPOINT_SIZE,
>   .bDescriptorType =  USB_DT_ENDPOINT,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Thu, Sep 22, 2016 at 12:02 AM, Sriram Dash  wrote:
>>From: Arnd Bergmann [mailto:a...@arndb.de]
>>On Wednesday, September 21, 2016 11:43:59 AM CEST Sriram Dash wrote:
>>> >From: Arnd Bergmann [mailto:a...@arndb.de] On Wednesday, September
>>> >21, 2016 11:06:47 AM CEST Sriram Dash wrote:
>>>
>>> ==
>>> From 8b0dea1513e9e73a11dcfa802ddc71cca11d66f8 Mon Sep 17 00:00:00 2001
>>> From: Sriram Dash 
>>> Date: Wed, 21 Sep 2016 11:39:30 +0530
>>> Subject: [PATCH] usb: xhci: Fix the patch inherit dma configuration
>>> from  parent dev
>>>
>>> Fixes the patch https://patchwork.kernel.org/patch/9319527/
>>> ("usb: dwc3: host: inherit dma configuration from parent dev").
>>>
>>> Signed-off-by: Sriram Dash 
>>> ---
>>>  drivers/usb/host/xhci-mem.c | 12 ++--
>>>  drivers/usb/host/xhci.c | 20 ++--
>>>  2 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index 6afe323..79608df 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>
>>All the changes you did to this file seem fine, I completely missed that part.
>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
>>> 01d96c9..9a1ff09 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>
> Yes. Some sanity is required over this patch.

Hi Sriram,

Have you been able to do the sanity check on the patch?  I will be
good to have the formal patch submitted for integration as soon as
possible because the dwc3 USB functionality has been broken for a
while in upstream kernel.

Regards,
Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Fri, Sep 2, 2016 at 5:43 AM, Arnd Bergmann <a...@arndb.de> wrote:
> On Thursday, September 1, 2016 5:14:28 PM CEST Leo Li wrote:
>>
>> Hi Felipe and Arnd,
>>
>> It has been a while since the last response to this discussion, but we
>> haven't reached an agreement yet!  Can we get to a conclusion on if it
>> is valid to create child platform device for abstraction purpose?  If
>> yes, can this child device do DMA by itself?
>
> I'd say it's no problem for a driver to create child devices in order
> to represent different aspects of a device, but you should not rely on
> those devices working when used with the dma-mapping interfaces.
>
> This used to be simpler back when we could configure the kernel for
> only one SoC platform at a time, and the platforms could provide their
> own overrides for the dma-mapping interfaces. These days, we rely on
> firmware or bootloader to describe various aspects of how DMA is done,
> so you can't assume that passing a device without an of_node pointer
> or ACPI data into those functions will do the right thing.

Can we use the firmware or bootloader information to provide the
default dma-mapping attributes for devices that doesn't have an
of_node pointer or ACPI data?  This will at least restore what we had
previously provided .  I'm concerned that changing all the drivers
that are creating child device will be a big effort.  Like I mentioned
in another thread, there are many instances of platform_device_add()
under the drivers/ directory.

- Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Leo Li]

On Thu, Apr 28, 2016 at 9:27 AM, Felipe Balbi  wrote:
>
> Hi,
>
> Arnd Bergmann  writes:
>> On Thursday 28 April 2016 15:16:12 Russell King - ARM Linux wrote:
>>> On Thu, Apr 28, 2016 at 09:37:08AM +0300, Felipe Balbi wrote:
>>> >
>>> > Hi,
>>> >
>>> > Arnd Bergmann  writes:
>>> > >pointer and pass that in platform_data. This is really easy, it's
>>> >
>>> > Sorry but passing a struct device pointer in platform_data is
>>> > ridiculous. Not to mention that, as I said before, we can't assume which
>>> > device to pass to xhci_plat in the first place. It might be dwc->dev and
>>> > it might be dwc->dev->parent.
>>>
>>> +1.  Passing an unref-counted struct device through platform data is
>>> totally mad, Arnd you're off your rocker if you think that's a good
>>> idea.  What's more is that there's no way to properly refcount the
>>> thing.
>>
>> It's the parent device (or NULL), there is no way it can ever go away as
>> it's already refcounted through the device subsystem by the creation
>> of the child device.
>
> you're assuming that based on what we have today. We could get into a
> situation where we need to use a completely unrelated device and the
> problem exists again.
>
>> I do realize that it's a hack, but the idea is to get rid of that
>> as soon as possibly by fixing the way the xhci device is probe so
>> we no longer need to fake a platform_device as the child here and
>> can just use the device itself.
>
> okay, let me try to be extra clear here:
>
> We will *not* remove the extra platform_device because it actually
> *does* exist and helps me hide/abstract a bunch of details and make
> assumptions about order of certain events. We have already gone through
> that in the past when I explained why I wrote dwc3 the way it is; if you
> need a refresher, there are mailing list archives for that.
>
> Moreover, this same problem exists for anything under drivers/mfd. It
> just so happens that they're usually some i2c or spi device which don't
> do DMA by themselves.

Hi Felipe and Arnd,

It has been a while since the last response to this discussion, but we
haven't reached an agreement yet!  Can we get to a conclusion on if it
is valid to create child platform device for abstraction purpose?  If
yes, can this child device do DMA by itself?

- Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers: usb: dwc3 : Configure DMA properties and ops from DT

[Leo Li]

On Thu, May 26, 2016 at 3:30 AM, Felipe Balbi
<felipe.ba...@linux.intel.com> wrote:
>
> Hi,
>
> Leo Li <pku@gmail.com> writes:
>>>> On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
>>>> to be able to do DMA allocations, so use the of_dma_configure() helper
>>>> to populate the dma properties and assign an appropriate dma_ops.
>>>>
>>>> Signed-off-by: Rajesh Bhagat <rajesh.bha...@nxp.com>
>>>> Reviewed-by: Yang-Leo Li <leoyang...@nxp.com>
>>>
>>> Cool, nxp is also using dwc3 :-) C'mon Rajesh, send us a glue layer :)
>>>
>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>> index c679f63..4d5b783 100644
>>>> --- a/drivers/usb/dwc3/host.c
>>>> +++ b/drivers/usb/dwc3/host.c
>>>> @@ -17,6 +17,7 @@
>>>>
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>>
>>>>  #include "core.h"
>>>>
>>>> @@ -32,6 +33,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>   return -ENOMEM;
>>>>   }
>>>>
>>>> + if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node)
>>>> + of_dma_configure(>dev, dwc->dev->of_node);
>>>
>>> okay, so we have a long discussion about this going on. You can catch up
>>> with it starting here:
>>>
>>> http://marc.info/?i=1461612094-30939-1-git-send-email-grygorii.stras...@ti.com
>>>
>>> At least for now, this patch will be applied. We need to have a better
>>> solution for this, one that helps not only DT platforms.
>>
>> Balbi,
>>
>> Has the patch from Grygorii been applied?  I don't see it in the
>> mainline tree yet.  Without fix, the dwc3 driver will fail for all
>> ARM64 SoCs.
>
> right, it's still broken. But we don't want something that fixes only
> OF, right? dwc3 is also broken for PCI when IOMMU is enabled. It breaks
> for the same reasons.
>
> We really need a way to inherit DMA bits from parent device here.

I agree with your proposal, but the original discussion seems to be on
halt right now.  If it need more time to get to an agreement on proper
fix, probably it's better to have a temporary fix right now to make
the driver working again.

Regards,
Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers: usb: dwc3 : Configure DMA properties and ops from DT

[Leo Li]

On Wed, May 4, 2016 at 2:57 AM, Felipe Balbi
<felipe.ba...@linux.intel.com> wrote:
>
> Hi,
>
> Rajesh Bhagat <rajesh.bha...@nxp.com> writes:
>> On certain platforms (e.g. ARM64) the dma_ops needs to be explicitly set
>> to be able to do DMA allocations, so use the of_dma_configure() helper
>> to populate the dma properties and assign an appropriate dma_ops.
>>
>> Signed-off-by: Rajesh Bhagat <rajesh.bha...@nxp.com>
>> Reviewed-by: Yang-Leo Li <leoyang...@nxp.com>
>
> Cool, nxp is also using dwc3 :-) C'mon Rajesh, send us a glue layer :)
>
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index c679f63..4d5b783 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -17,6 +17,7 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "core.h"
>>
>> @@ -32,6 +33,9 @@ int dwc3_host_init(struct dwc3 *dwc)
>>   return -ENOMEM;
>>   }
>>
>> + if (IS_ENABLED(CONFIG_OF) && dwc->dev->of_node)
>> + of_dma_configure(>dev, dwc->dev->of_node);
>
> okay, so we have a long discussion about this going on. You can catch up
> with it starting here:
>
> http://marc.info/?i=1461612094-30939-1-git-send-email-grygorii.stras...@ti.com
>
> At least for now, this patch will be applied. We need to have a better
> solution for this, one that helps not only DT platforms.

Balbi,

Has the patch from Grygorii been applied?  I don't see it in the
mainline tree yet.  Without fix, the dwc3 driver will fail for all
ARM64 SoCs.

Regards,
Leo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html