Re: [PATCH v2] usb: gadget: function: printer: avoid wrong list handling in printer_write()

[Felipe Balbi]

Hi Greg,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
> When printer_write() calls usb_ep_queue(), a udc driver (e.g.
> renesas_usbhs driver) may call usb_gadget_giveback_request() in
> the udc .queue ops immediately. Then, printer_write() calls
> list_add(>list, >tx_reqs_active) wrongly. After that,
> if we do unbind the printer driver, WARN_ON() happens in
> printer_func_unbind() because the list entry is not removed.
>
> So, this patch moves list_add(>list, >tx_reqs_active)
> calling before usb_ep_queue().
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
> Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

I'm not sure if you're still taking bug fixes for current -rc cycle, but
if you are, please take this directly. If you're done with v4.17, please
apply this on top of my pull request which I sent an hour or so ago.

Thanks

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
> Hi,
>
>> From: Felipe Balbi, Sent: Monday, May 21, 2018 5:05 PM
> 
>> seems like it would be better to just move this like before
>> usb_ep_queue():
>> 
>> modified   drivers/usb/gadget/function/f_printer.c
>> @@ -631,19 +631,19 @@ printer_write(struct file *fd, const char __user *buf, 
>> size_t len, loff_t *ptr)
>>  return -EAGAIN;
>>  }
>> 
>> +list_add(>list, >tx_reqs_active);
>> +
>>  /* here, we unlock, and only unlock, to avoid deadlock. */
>>  spin_unlock(>lock);
>>  value = usb_ep_queue(dev->in_ep, req, GFP_ATOMIC);
>>  spin_lock(>lock);
>>  if (value) {
>> +list_del(>list);
>>  list_add(>list, >tx_reqs);
>>  spin_unlock_irqrestore(>lock, flags);
>>  mutex_unlock(>lock_printer_io);
>>  return -EAGAIN;
>>  }
>> -
>> -list_add(>list, >tx_reqs_active);
>> -
>>  }
>> 
>>  spin_unlock_irqrestore(>lock, flags);
>> 
>> --
>
> Thank you very much for your patch! This could resolve the issue.
> So, should I submit this your patch as your author?

you can send it with your authorship, it's totally fine :-)

You can also add my:

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

thanks

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:

> Hi,
>
>> From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM
>> 
>> Hi,
>> 
>> Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
>> > The usb_ep_queue() in printer_write() is possible to call req->complete().
>> > In that case, since tx_complete() calls list_add(>list, 
>> > >tx_reqs),
>> > printer_write() should not call list_add(>list, >tx_reqs_active)
>> > because the transfer has already finished. So, this patch checks
>> > the condition of req->list before adding the list in printer_write().
>> >
>> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
>> > ---
>> >  This issue can be caused by renesas_usbhs udc driver. I'm not sure
>> >  this patch is acceptable or not. So, I marked RFC on this patch.
>> 
>> can you explain this a little more? How do you trigger the problem?
>
> Sure. If printer_write() called usb_ep_queue() with 63 bytes or less data,
> the renesas_usbhs udc driver transfers data as PIO. In this case, the udc
> driver calls usb_gadget_giveback_reuqest() in .queue ops (usbhsg_ep_queue())
> immediately. Then, printer_write() calls list_add(>list, 
> >tx_reqs_active); wrongly.
> After that, if we do rmmod g_printer, 
> WARN_ON(!list_empty(>tx_reqs_active); happens in
> printer_func_unbind() because the list entry is not removed.
>
> < Reference: calltrace (very long though...) >
>   usb_ep_queue(...);  at f_printer.c / printer_write()
>   1-> usbhsg_ep_queue();  at renesas_usbhs/mod_gadget.c
>2-> usbhsg_queue_push();
> 3-> usbhs_pkt_start();at renesas_usbhs/fifo.c
>  4-> usbhsf_pkt_handler();
>   5-> func() = usbhsf_dma_prepare_push();
>6-> goto usbhsf_pio_prepare_push; // Because len is 63
> 7-> usbhsf_pio_prepare_push();
>  8-> usbhsf_pio_try_push();
> 5-> done() = usbhsg_queue_done(); at renesas_usbhs/mod_gadget.c
>6-> __usbsg_queue_pop();
> 7-> usb_gadget_giveback_reuqest();
>  8-> tx_complete();   at f_printer.c
> 9-> list_del_init(>list);
> 9-> list_add(>list, >tx_reqs);
>   list_add(>list, >tx_reqs_active); // Even if the 
> transaction already finished, this driver is possible to add the list to 
> "active".

seems like it would be better to just move this like before
usb_ep_queue():

modified   drivers/usb/gadget/function/f_printer.c
@@ -631,19 +631,19 @@ printer_write(struct file *fd, const char __user *buf, 
size_t len, loff_t *ptr)
return -EAGAIN;
}
 
+   list_add(>list, >tx_reqs_active);
+
/* here, we unlock, and only unlock, to avoid deadlock. */
spin_unlock(>lock);
value = usb_ep_queue(dev->in_ep, req, GFP_ATOMIC);
spin_lock(>lock);
if (value) {
+   list_del(>list);
list_add(>list, >tx_reqs);
spin_unlock_irqrestore(>lock, flags);
mutex_unlock(>lock_printer_io);
return -EAGAIN;
}
-
-   list_add(>list, >tx_reqs_active);
-
}
 
spin_unlock_irqrestore(>lock, flags);

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> The usb_ep_queue() in printer_write() is possible to call req->complete().
> In that case, since tx_complete() calls list_add(>list, >tx_reqs),
> printer_write() should not call list_add(>list, >tx_reqs_active)
> because the transfer has already finished. So, this patch checks
> the condition of req->list before adding the list in printer_write().
>
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  This issue can be caused by renesas_usbhs udc driver. I'm not sure
>  this patch is acceptable or not. So, I marked RFC on this patch.

can you explain this a little more? How do you trigger the problem?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: renesas_usbhs: Add compatible string for r8a7743/5

[Felipe Balbi]

Hi,

Biju Das <biju@bp.renesas.com> writes:
> Hi,
>
>> -Original Message-
>> From: devicetree-ow...@vger.kernel.org [mailto:devicetree-
>> ow...@vger.kernel.org] On Behalf Of Felipe Balbi
>> Sent: 17 October 2017 09:26
>> To: Biju Das <biju@bp.renesas.com>; Greg Kroah-Hartman
>> <gre...@linuxfoundation.org>; Rob Herring <robh...@kernel.org>; Mark
>> Rutland <mark.rutl...@arm.com>
>> Cc: Simon Horman <ho...@verge.net.au>; Chris Paterson
>> <chris.paters...@renesas.com>; Fabrizio Castro
>> <fabrizio.cas...@bp.renesas.com>; devicet...@vger.kernel.org; linux-renesas-
>> s...@vger.kernel.org; linux-...@vger.kernel.org; Biju Das
>> <biju@bp.renesas.com>
>> Subject: Re: [PATCH] usb: renesas_usbhs: Add compatible string for r8a7743/5
>>
>>
>> Hi,
>>
>> Biju Das <biju@bp.renesas.com> writes:
>> > This patch adds support for r8a7743/5 SoC.  The Renesas RZ/G1[ME]
>> > (R8A7743/5) usbhs is identical to the R-Car Gen2 family.
>> >
>> > This doesn't change the driver, so it does nothing by itself.  But it
>> > does mean that checkpatch won't complain about a future patch that
>> > adds "renesas,usbhs-r8a7743" or "renesas,usbhs-r8a7745" to a DT, which
>> > helps ensure that shipped DTs use documented compatibility strings.
>> >
>> > Signed-off-by: Biju Das <biju@bp.renesas.com>
>> > ---
>> > This patch is tested against Linux next tag next-20170929
>
>> care to rebase on my testing/next? It fails to apply.
>
> Sorry for the trouble.
>
> I have already send a V2 for fixing this.
>
> [v2] usb: renesas_usbhs: Add compatible string for r8a7743/5
> https://patchwork.kernel.org/patch/9990063/
>
> I have rebased this patch on my testing/next and confirms it apply cleanly.

thank you, now applied :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: renesas_usbhs: Add compatible string for r8a7743/5

[Felipe Balbi]

Hi,

Biju Das  writes:
> This patch adds support for r8a7743/5 SoC.  The Renesas RZ/G1[ME]
> (R8A7743/5) usbhs is identical to the R-Car Gen2 family.
>
> This doesn't change the driver, so it does nothing by itself.  But it does
> mean that checkpatch won't complain about a future patch that adds
> "renesas,usbhs-r8a7743" or "renesas,usbhs-r8a7745" to a DT, which helps
> ensure that shipped DTs use documented compatibility strings.
>
> Signed-off-by: Biju Das 
> ---
> This patch is tested against Linux next tag next-20170929

care to rebase on my testing/next? It fails to apply.

While doing that, please collect all Acks given.

8<

checking file Documentation/devicetree/bindings/usb/renesas_usbhs.txt
Hunk #2 FAILED at 12.
1 out of 2 hunks FAILED

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] extcon: Split out extcon header file for consumer and provider device

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> The extcon has two type of extcon devices as following.
> - 'extcon provider deivce' adds new extcon device and detect the
>state/properties of external connector. Also, it notifies the
>state/properties to the extcon consumer device.
> - 'extcon consumer device' gets the change state/properties
>from extcon provider device.
> Prior to that, include/linux/extcon.h contains all exported API for
> both provider and consumer device driver. To clarify the meaning of
> header file and to remove the wrong use-case on consumer device,
> this patch separates into extcon.h and extcon-provider.h.
>
> [Description for include/linux/{extcon.h|extcon-provider.h}]
> - extcon.h includes the extcon API and data structure for extcon consumer
>   device driver. This header file contains the following APIs:
>   : Register/unregister the notifier to catch the change of extcon device
>   : Get the extcon device instance
>   : Get the extcon device name
>   : Get the state of each external connector
>   : Get the property value of each external connector
>   : Get the property capability of each external connector
>
> - extcon-provider.h includes the extcon API and data structure for extcon
>   provider device driver. This header file contains the following APIs:
>   : Include 'include/linux/extcon.h'
>   : Allocate the memory for extcon device instance
>   : Register/unregister extcon device
>   : Set the state of each external connector
>   : Set the property value of each external connector
>   : Set the property capability of each external connector
>
> Cc: Felipe Balbi <ba...@kernel.org>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi

Re: [PATCH v2 3/3] usb: gadget: udc: renesas_usb3: add support for R-Car M3-W

[Felipe Balbi]

Hi,

Rob Herring  writes:
> On Fri, Aug 04, 2017 at 11:16:58AM +0900, Yoshihiro Shimoda wrote:
>> This patch adds support for R-Car M3-W. This patch also adds R-Car
>> Gen3 generic version's compatible and changes ".compatible" in
>> the usb3_of_match from "renesas,r8a7796-usb3-peri" to
>> "renesas,rcar-gen3-usb3-peri".
>> 
>> Signed-off-by: Yoshihiro Shimoda 
>> ---
>>  Documentation/devicetree/bindings/usb/renesas_usb3.txt | 16 +---
>>  drivers/usb/gadget/udc/renesas_usb3.c  |  2 +-
>>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> Binding looks fine, but...
>
>> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c 
>> b/drivers/usb/gadget/udc/renesas_usb3.c
>> index aa2b185..b1e166c 100644
>> --- a/drivers/usb/gadget/udc/renesas_usb3.c
>> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
>> @@ -2503,7 +2503,7 @@ static void renesas_usb3_init_ram(struct renesas_usb3 
>> *usb3, struct device *dev,
>>  
>>  static const struct of_device_id usb3_of_match[] = {
>>  {
>> -.compatible = "renesas,r8a7795-usb3-peri",
>> +.compatible = "renesas,rcar-gen3-usb3-peri",
>
> You need to keep the existing string for compatibility with existing 
> dtbs.

I've fixed it up locally:

diff --git a/drivers/usb/gadget/udc/renesas_usb3.c 
b/drivers/usb/gadget/udc/renesas_usb3.c
index ff69f4645b7c..16ceb445bee8 100644
--- a/drivers/usb/gadget/udc/renesas_usb3.c
+++ b/drivers/usb/gadget/udc/renesas_usb3.c
@@ -2512,6 +2512,10 @@ static const struct of_device_id usb3_of_match[] = {
.compatible = "renesas,r8a7795-usb3-peri",
.data = _usb3_priv_gen3,
},
+   {
+   .compatible = "renesas,rcar-gen3-usb3-peri",
+   .data = _usb3_priv_gen3,
+   },
{ },
 };
 MODULE_DEVICE_TABLE(of, usb3_of_match);


-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH 0/5] usb: gadget: udc: renesas_usb3: add DMAC support

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> Hi Felipe,
>
>> -Original Message-
>> From: Yoshihiro Shimoda
>> Sent: Wednesday, April 26, 2017 8:50 PM
>> 
>> This patch set is based on the latest Feribe's usb.git / testing/next branch
>> (the commit id = 28ea6be01e2cf244c461a40c8e9593816f894412.)
>
> I'm afraid but, would you review this patch set?
> This patch set can be applied on the today's testing/next branch.

I was thinking that we need the fixes during the -rc cycle. No? In that
case, we need to get rid of the dependencies between 1-4 and patch 5.

patches 1-4 are in my testing/fixes now.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 3/3] usb: gadget: udc: renesas_usb3: add support for usb role swap

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> This patch adds support for usb role swap via sysfs "role".
>
> For example:
>  1) Connect a usb cable using 2 Salvator-X boards.
>   - For A-Device, the cable is connected to CN11 (USB3.0 ch0).
>   - For B-Device, the cable is connected to CN9 (USB2.0 ch0).
>  2) On A-Device, you input the following command:
>   # echo peripheral > /sys/devices/platform/soc/ee02.usb/role
>  3) On B-Device, you input the following command:
>   # echo host > /sys/devices/platform/soc/ee080200.usb-phy/role
>
> Then, the A-Device acts as a peripheral and the B-Device acts as
> a host. Please note that A-Device must input the following command
> if you want the board to act as a host again.
>  # echo host > /sys/devices/platform/soc/ee02.usb/role
>
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  .../ABI/testing/sysfs-platform-renesas_usb3| 15 ++
>  drivers/usb/gadget/udc/renesas_usb3.c  | 56 
> ++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-renesas_usb3
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-renesas_usb3 
> b/Documentation/ABI/testing/sysfs-platform-renesas_usb3
> new file mode 100644
> index 000..1f63190
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-renesas_usb3
> @@ -0,0 +1,15 @@
> +What:/sys/devices/platform//role

I have one question here. This seems to imply that platform devices have
a "role" file which is not true for all platforms devices. I really
don't have a suggestion as to how this could/should be written out,
though :-s

Greg, any suggestions?

(keeping patch below for reference only)

> +Date:March 2017
> +KernelVersion:   4.13
> +Contact: Yoshihiro Shimoda 
> +Description:
> + This file can be read and write.
> + The file can show/change the drd mode of usb.
> +
> + Write the following string to change the mode:
> +  "host" - switching mode from peripheral to host.
> +  "peripheral" - switching mode from host to peripheral.
> +
> + Read the file, then it shows the following strings:
> +  "host" - The mode is host now.
> +  "peripheral" - The mode is peripheral now.
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c 
> b/drivers/usb/gadget/udc/renesas_usb3.c
> index a1e79fc..5a2d845 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -372,6 +372,11 @@ static void usb3_disable_pipe_irq(struct renesas_usb3 
> *usb3, int num)
>   usb3_clear_bit(usb3, USB_INT_2_PIPE(num), USB3_USB_INT_ENA_2);
>  }
>  
> +static bool usb3_is_host(struct renesas_usb3 *usb3)
> +{
> + return !(usb3_read(usb3, USB3_DRD_CON) & DRD_CON_PERI_CON);
> +}
> +
>  static void usb3_init_axi_bridge(struct renesas_usb3 *usb3)
>  {
>   /* Set AXI_INT */
> @@ -576,8 +581,14 @@ static void usb3_vbus_out(struct renesas_usb3 *usb3, 
> bool enable)
>  
>  static void usb3_mode_config(struct renesas_usb3 *usb3, bool host, bool 
> a_dev)
>  {
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
>   usb3_set_mode(usb3, host);
>   usb3_vbus_out(usb3, a_dev);
> + if (!host && a_dev) /* for A-Peripheral */
> + usb3_connect(usb3);
> + spin_unlock_irqrestore(>lock, flags);
>  }
>  
>  static bool usb3_is_a_device(struct renesas_usb3 *usb3)
> @@ -1837,11 +1848,49 @@ static int renesas_usb3_set_selfpowered(struct 
> usb_gadget *gadget, int is_self)
>   .set_selfpowered= renesas_usb3_set_selfpowered,
>  };
>  
> +static ssize_t role_store(struct device *dev, struct device_attribute *attr,
> +   const char *buf, size_t count)
> +{
> + struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> + bool new_mode_is_host;
> +
> + if (!usb3->driver)
> + return -ENODEV;
> +
> + if (!strncmp(buf, "host", strlen("host")))
> + new_mode_is_host = true;
> + else if (!strncmp(buf, "peripheral", strlen("peripheral")))
> + new_mode_is_host = false;
> + else
> + return -EINVAL;
> +
> + if (new_mode_is_host == usb3_is_host(usb3))
> + return -EINVAL;
> +
> + usb3_mode_config(usb3, new_mode_is_host, usb3_is_a_device(usb3));
> +
> + return count;
> +}
> +
> +static ssize_t role_show(struct device *dev, struct device_attribute *attr,
> +  char *buf)
> +{
> + struct renesas_usb3 *usb3 = dev_get_drvdata(dev);
> +
> + if (!usb3->driver)
> + return -ENODEV;
> +
> + return sprintf(buf, "%s\n", usb3_is_host(usb3) ? "host" : "peripheral");
> +}
> +static DEVICE_ATTR_RW(role);
> +
>  /*--- platform_driver */
>  static int 

RE: [PATCH 1/4] usb: gadget: udc: renesas_usb3: add sysfs "role" to set "b-device" mode

[Felipe Balbi]

Yoshihiro-san

Yoshihiro Shimoda  writes:
>> >> Yoshihiro Shimoda  writes:
>> >> > Sadly, to change the role ("host" and "peripheral") of USB3.0 DRD
>> >> > controller on R-Car Gen3, software has to set the DRD_CON register
>> >> > which is included in a usb3.0 peripheral controler's register.
>> >> > To simply implementation, the previous code always set peripheral mode
>> >> > as hardcoded. However, to support usb role swap in the future,
>> >> > the hardcoded is not good. So, this patch adds sysfs "role" to set
>> >> > the mode by a user.
>> >> > After applied this patch, since the DRD controller will act as host
>> >> > mode after probed, a user needs to change the mode via the sysfs.
>> >> >
>> >> > Signed-off-by: Yoshihiro Shimoda 
>> >>
>> >> in patch 3, you add extcon to monitor ID and VBUS pins. Do you really
>> >> need this sysfs file at all?
>> >
>> > Yes. This is because this "role" sysfs file cannot monitor the ID and VBUS 
>> > pins.
>> > The "role" only monitors the mode which is "host" or "peripheral".
>> 
>> Right, I understand it can't monitor the state of ID/VBUS, but do you
>> have a usecase for that file that's not covered by the other patches in
>> the series? :-)
>
> Thank you for the comment. Since my English skill is not good,
> I don't understand your question.

oh, that's alright. I'll try to explain:

I'm asking if you have any situation where you need userspace to change
the role manually. I understand it's a very nice feature to have during
development because it frees us from searching for "the right cable";
but if the only application of this sysfs file is durinb development,
then perhaps it should be exposed using debugfs instead ;-)

Hopefully that's easier to understand now.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH 1/4] usb: gadget: udc: renesas_usb3: add sysfs "role" to set "b-device" mode

[Felipe Balbi]

Hi,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
> Hi Felipe-san,
>
>> From: Felipe Balbi, Sent: Tuesday, March 28, 2017 9:06 PM
>> 
>> Hi,
>> 
>> Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
>> > Sadly, to change the role ("host" and "peripheral") of USB3.0 DRD
>> > controller on R-Car Gen3, software has to set the DRD_CON register
>> > which is included in a usb3.0 peripheral controler's register.
>> > To simply implementation, the previous code always set peripheral mode
>> > as hardcoded. However, to support usb role swap in the future,
>> > the hardcoded is not good. So, this patch adds sysfs "role" to set
>> > the mode by a user.
>> > After applied this patch, since the DRD controller will act as host
>> > mode after probed, a user needs to change the mode via the sysfs.
>> >
>> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
>> 
>> in patch 3, you add extcon to monitor ID and VBUS pins. Do you really
>> need this sysfs file at all?
>
> Yes. This is because this "role" sysfs file cannot monitor the ID and VBUS 
> pins.
> The "role" only monitors the mode which is "host" or "peripheral".

Right, I understand it can't monitor the state of ID/VBUS, but do you
have a usecase for that file that's not covered by the other patches in
the series? :-)

If you want to force renesas controller to peripheral mode, then
shouldn't you use dr-mode device_property() instead?

-- 
balbi

Re: [PATCH v2 1/6] usb: dwc3: omap: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>
> Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

doesn't apply to testing/next. Please rebase

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 06/12] usb: dwc3: omap: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> Hi Felipe,
>
> On 2016년 11월 30일 19:36, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Chanwoo Choi <cw00.c...@samsung.com> writes:
>>> This patch uses the resource-managed extcon API for 
>>> extcon_register_notifier()
>>> and replaces the deprecated extcon API as following:
>>> - extcon_get_cable_state_() -> extcon_get_state()
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>
>> 
>> Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>> 
>
> Thanks for your review.
>
> Each patch has no any dependency among patches.
> So, If possible, could you pick the patch6/8/9/10/11/12 on your tree?

my tree is closed for v4.10, I can pick it up for v4.11

-- 
balbi

Re: [PATCH 12/12] usb: renesas_usbhs: Replace the deprecated extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 10/12] usb: phy: qcom-8x16-usb: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 11/12] usb: phy: tahvo: Replace the deprecated extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch replaces the deprecated extcon API as following:
> - extcon_set_cable_state_() -> extcon_set_state_sync()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 09/12] usb: phy: omap-otg: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 08/12] usb: phy: msm: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 06/12] usb: dwc3: omap: Replace the extcon API

[Felipe Balbi]

Hi,

Chanwoo Choi <cw00.c...@samsung.com> writes:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
>
> Signed-off-by: Chanwoo Choi <cw00.c...@samsung.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 00/26] constify local structures

[Felipe Balbi]

Hi,

Jarkko Sakkinen  writes:
> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>> 
>> 
>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>> 
>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>> > > Constify local structures.
>> > >
>> > > The semantic patch that makes this change is as follows:
>> > > (http://coccinelle.lip6.fr/)
>> >
>> > Just my two cents but:
>> >
>> > 1. You *can* use a static analysis too to find bugs or other issues.
>> > 2. However, you should manually do the commits and proper commit
>> >messages to subsystems based on your findings. And I generally think
>> >that if one contributes code one should also at least smoke test changes
>> >somehow.
>> >
>> > I don't know if I'm alone with my opinion. I just think that one should
>> > also do the analysis part and not blindly create and submit patches.
>> 
>> All of the patches are compile tested.  And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.

Dude, Julia has been doing semantic patching for years already and
nobody has raised any concerns so far. There's already an expectation
that Coccinelle *works* and Julia's sematic patches are sound.

Besides, adding 'const' is something that causes virtually no functional
changes to the point that build-testing is really all you need. Any
problems caused by adding 'const' to a definition will be seen by build
errors or warnings.

Really, just stop with the pointless discussion and go read a bit about
Coccinelle and what semantic patches are giving you. The work done by
Julia and her peers are INRIA have measurable benefits.

You're really making a thunderstorm in a glass of water.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH 0/2] usb: renesas_usbhs: fix issues on specific situations

[Felipe Balbi]

Yoshihiro Shimoda  writes:

> Hi Felipe,
>
> Would you review this patch set?

both in my queue.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Felipe Balbi]

Hi,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
>> From: Felipe Balbi
>> Sent: Wednesday, June 01, 2016 4:01 PM
>> 
>> Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
>> 
>> > I'm afraid but I found a regression of xhci-rcar in v4.7-rc1.
>> > This regression is caused by the following commit:
>> >
>> > commit b1c127ae990bccf0187d741c1695a61e54de1943
>> > Author: Felipe Balbi <felipe.ba...@linux.intel.com>
>> > Date:   Fri Apr 22 13:17:16 2016 +0300
>> >
>> > usb: host: xhci: plat: make use of new methods in xhci_plat_priv
>> >
>> > Now that the code has been refactored enough,
>> > switching over to using ->plat_start() and
>> > ->init_quirk() becomes a very simple patch.
>> >
>> > After this patch, there are no further uses for
>> >     xhci_plat_type_is() which will be removed in a
>> > follow-up patch.
>> >
>> > Acked-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
>> > Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>> > Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com>
>> > Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>> >
>> > < Overview >
>> > The regression is the quirks flag "XHCI_NO_64BIT_SUPPORT" will be 
>> > overwritten
>> > by xhci_gen_setup(). Then, the driver will not work correctly.
>> >
>> > < Detail >
>> > Since the previous code will do the following, the quirks flag can be set:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >   -> xhci->quirks = quirks;
>> >-> get_quirks() [This is xhci_plat_quirks]
>> > -> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>> >
>> > However, after we applied the patch above, the quirks will disappear:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_priv_init_quirk();
>> >   -> xhci_rcar_init_quirk();
>> >-> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >-> xhci->quirks = quirks; <- here
>> > -> get_quirks() [This is xhci_plat_quirks]
>> >
>> > So, I submitted incremental patches to resolve this issue like the 
>> > followings:
>> >
>> > xhci_plat_setup()
>> >  -> xhci_priv_init_quirk();
>> >   -> xhci_rcar_init_quirk();
>> >   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>> >-> xhci->quirks = quirks;
>> > -> get_quirks() [This is xhci_plat_quirks]
>> >  -> xhci->quirks |= priv->quirks (XHCI_NO_64BIT_SUPPORT)
>> 
>> isn't the following enough?
>> 
>> @@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>> xhci_get_quirks_t get_quirks)
>>  xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
>>  xhci_print_registers(xhci);
>> 
>> -xhci->quirks = quirks;
>> +xhci->quirks |= quirks;
>> 
>>  get_quirks(dev, xhci);
>
> Thank you for the comment!
> You're correct. This also can resolve the issue.
> Do you prefer such a simple patch?
> At least, I prefer such a simple patch :)

I'll defer that to Mathias :-)

> Why I wrote this patch set is I thought I should implement similar
> flow before regression.

understood :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 0/2] usb: host: xhci: rcar: fix the quirks setting of XHCI_NO_64BIT_SUPPORT

[Felipe Balbi]

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:

> I'm afraid but I found a regression of xhci-rcar in v4.7-rc1.
> This regression is caused by the following commit:
>
> commit b1c127ae990bccf0187d741c1695a61e54de1943
> Author: Felipe Balbi <felipe.ba...@linux.intel.com>
> Date:   Fri Apr 22 13:17:16 2016 +0300
>
> usb: host: xhci: plat: make use of new methods in xhci_plat_priv
> 
> Now that the code has been refactored enough,
> switching over to using ->plat_start() and
> ->init_quirk() becomes a very simple patch.
> 
> After this patch, there are no further uses for
> xhci_plat_type_is() which will be removed in a
> follow-up patch.
> 
> Acked-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.ny...@linux.intel.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>
> < Overview >
> The regression is the quirks flag "XHCI_NO_64BIT_SUPPORT" will be overwritten
> by xhci_gen_setup(). Then, the driver will not work correctly.
>
> < Detail >
> Since the previous code will do the following, the quirks flag can be set:
>
> xhci_plat_setup()
>  -> xhci_gen_setup(hcd, xhci_plat_quirks);
>   -> xhci->quirks = quirks;
>-> get_quirks() [This is xhci_plat_quirks]
> -> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>
> However, after we applied the patch above, the quirks will disappear:
>
> xhci_plat_setup()
>  -> xhci_priv_init_quirk();
>   -> xhci_rcar_init_quirk();
>-> xhci->quirks |= XHCI_NO_64BIT_SUPPORT
>   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>-> xhci->quirks = quirks; <- here
> -> get_quirks() [This is xhci_plat_quirks]
>
> So, I submitted incremental patches to resolve this issue like the followings:
>
> xhci_plat_setup()
>  -> xhci_priv_init_quirk();
>   -> xhci_rcar_init_quirk();
>   -> xhci_gen_setup(hcd, xhci_plat_quirks);
>-> xhci->quirks = quirks;
> -> get_quirks() [This is xhci_plat_quirks]
>  -> xhci->quirks |= priv->quirks (XHCI_NO_64BIT_SUPPORT)

isn't the following enough?

@@ -4886,7 +4886,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
xhci->hcc_params2 = readl(>cap_regs->hcc_params2);
xhci_print_registers(xhci);
 
-   xhci->quirks = quirks;
+   xhci->quirks |= quirks;
 
get_quirks(dev, xhci);
 
-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
> The firmware of R-Car USB 3.0 host controller will control the reset.
> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
> is not set), the reset of USB 3.0 host controller doesn't work
> correctly. Then, the host controller will cause long wait in
> xhci_reset() because the CMD_RESET bit of op_regs->command is not
> cleared for 10 seconds.
>
> So, this patch modifies the Kconfig to enable both CONFIG_USB_XHCI_PLATFORM
> and CONFIG_USB_XHCI_RCAR.
>
> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 
> xHCI controllers)
> Cc: <sta...@vger.kernel.org> # v3.17+
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>

looks good to me, thanks :)

Reviewed-by: Felipe Balbi <felipe.ba...@linux.intel.com>

> ---
>  Changes from v2:
>   - Modify the Kconfig instead of xhci-rcar.h
> http://www.spinics.net/lists/linux-usb/msg139681.html
> http://www.spinics.net/lists/linux-usb/msg139722.html
>
>  Changes from v1:
>   - Revise the commit log.
> http://www.spinics.net/lists/stable/msg130007.html
>
>  drivers/usb/host/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 3050b18..e9d4dde 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -35,6 +35,7 @@ config USB_XHCI_PCI
>  
>  config USB_XHCI_PLATFORM
>   tristate "Generic xHCI driver for a platform device"
> + select USB_XHCI_RCAR if ARCH_RENESAS
>   ---help---
> Adds an xHCI host driver for a generic platform device, which
> provides a memory space and an irq.
> @@ -63,7 +64,7 @@ config USB_XHCI_MVEBU
>  
>  config USB_XHCI_RCAR
>   tristate "xHCI support for Renesas R-Car SoCs"
> - select USB_XHCI_PLATFORM
> + depends on USB_XHCI_PLATFORM
>   depends on ARCH_RENESAS || COMPILE_TEST
>   ---help---
> Say 'Y' to enable the support for the xHCI host controller
> -- 
> 1.9.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

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
>> > Yoshihiro Shimoda  writes:
>> >> The firmware of R-Car USB 3.0 host controller will control the reset.
>> >> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
>> >> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
>> >> is not set), the reset of USB 3.0 host controller doesn't work
>> >> correctly. Then, the host controller will cause long wait in
>> >> xhci_reset() because the CMD_RESET bit of op_regs->command is not
>> >> cleared for 10 seconds.
>> >>
>> >> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h
>> >> to exit the probe function immediately.
>> >>
>> >> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 
>> >> and M2 xHCI controllers)
>> >> Cc:  # v3.17+
>> >> Signed-off-by: Yoshihiro Shimoda 
>> >> ---
>> >>  Changes from v1:
>> >>   - Revise the commit log.
>> >> (http://www.spinics.net/lists/stable/msg130007.html)
>> >>
>> >>  drivers/usb/host/xhci-rcar.h | 6 +-
>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
>> >> index 2941a25..2afed68 100644
>> >> --- a/drivers/usb/host/xhci-rcar.h
>> >> +++ b/drivers/usb/host/xhci-rcar.h
>> >> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)
>> >>
>> >>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)
>> >>  {
>> >> - return 0;
>> >> + /*
>> >> +  * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is
>> >> +  * disabled, this function fails.
>> >> +  */
>> >> + return -ENODEV;
>> >
>> > okay, if I understood correctly the thing is that you have a kernel
>> > built with XHCI platform support but without XHCI RCAR support. Then, if
>> > you run this kernel on RCAR board, you see this CMD_RESET problem,
>> > right?
>> >
>> > Isn't this pointing to the fact that xhci-plat.ko built without RCAR
>> > isn't exactly compatible with RCAR ?
>> >
>> > IOW:
>> >
>> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> > index 676ea458148b..3e39320564ce 100644
>> > --- a/drivers/usb/host/xhci-plat.c
>> > +++ b/drivers/usb/host/xhci-plat.c
>> > @@ -112,6 +112,7 @@ static const struct of_device_id usb_xhci_of_match[] = 
>> > {
>> > }, {
>> > .compatible = "marvell,armada-380-xhci",
>> > .data = _plat_marvell_armada,
>> > +#if IS_ENABLED(CONFIG_USB_XHCI_RCAR)
>> > }, {
>> > .compatible = "renesas,xhci-r8a7790",
>> > .data = _plat_renesas_rcar_gen2,
>> > @@ -130,6 +131,7 @@ static const struct of_device_id usb_xhci_of_match[] = 
>> > {
>> > }, {
>> > .compatible = "renesas,rcar-gen3-xhci",
>> > .data = _plat_renesas_rcar_gen3,
>> > +#endif
>> > },
>> > {},
>> >  };
>> >
>> > Rob, should we limit compatible flags like this ? Without
>> > CONFIG_USB_XHCI_RCAR, this driver won't work on RCAR but, as you can
>> > see, this driver might still work on other non-RCAR platforms.
>> >
>> > What's your take on this ?
>> 
>> We should fix this in kconfig to always enable the option when RCAR is
>> enabled IMO.
>
> I could fix this in kconfig like the followings:
>
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index f2bc5c3..905d1d2 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -46,6 +46,7 @@ menuconfig ARCH_RENESAS
>   select PINCTRL
>   select ARCH_REQUIRE_GPIOLIB
>   select ZONE_DMA if ARM_LPAE
> + select USB_XHCI_RCAR if USB_XHCI_HCD
>  
>  if ARCH_RENESAS
>  
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index efa77c1..010d2b7 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -105,6 +105,7 @@ config ARCH_RENESAS
>   select PM
>   select PM_GENERIC_DOMAINS
>   select RENESAS_IRQC
> + select USB_XHCI_RCAR if USB_XHCI_HCD
>   help
> This enables support for the ARMv8 based Renesas SoCs.
>
> If this is acceptable, I will send each patch to arm / arm64.

I'm okay with that, not sure what the ARM folks will think ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> The firmware of R-Car USB 3.0 host controller will control the reset.
> So, if the xhci driver doesn't do firmware downloading (e.g. kernel
> configuration is CONFIG_USB_XHCI_PLATFORM=y and CONFIG_USB_XHCI_RCAR
> is not set), the reset of USB 3.0 host controller doesn't work
> correctly. Then, the host controller will cause long wait in
> xhci_reset() because the CMD_RESET bit of op_regs->command is not
> cleared for 10 seconds.
>
> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h
> to exit the probe function immediately.
>
> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 
> xHCI controllers)
> Cc:  # v3.17+
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  Changes from v1:
>   - Revise the commit log.
> (http://www.spinics.net/lists/stable/msg130007.html)
>
>  drivers/usb/host/xhci-rcar.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
> index 2941a25..2afed68 100644
> --- a/drivers/usb/host/xhci-rcar.h
> +++ b/drivers/usb/host/xhci-rcar.h
> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)
>  
>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)
>  {
> - return 0;
> + /*
> +  * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is
> +  * disabled, this function fails.
> +  */
> + return -ENODEV;

okay, if I understood correctly the thing is that you have a kernel
built with XHCI platform support but without XHCI RCAR support. Then, if
you run this kernel on RCAR board, you see this CMD_RESET problem,
right?

Isn't this pointing to the fact that xhci-plat.ko built without RCAR
isn't exactly compatible with RCAR ?

IOW:

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 676ea458148b..3e39320564ce 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -112,6 +112,7 @@ static const struct of_device_id usb_xhci_of_match[] = {
}, {
.compatible = "marvell,armada-380-xhci",
.data = _plat_marvell_armada,
+#if IS_ENABLED(CONFIG_USB_XHCI_RCAR)
}, {
.compatible = "renesas,xhci-r8a7790",
.data = _plat_renesas_rcar_gen2,
@@ -130,6 +131,7 @@ static const struct of_device_id usb_xhci_of_match[] = {
}, {
.compatible = "renesas,rcar-gen3-xhci",
.data = _plat_renesas_rcar_gen3,
+#endif
},
{},
 };

Rob, should we limit compatible flags like this ? Without
CONFIG_USB_XHCI_RCAR, this driver won't work on RCAR but, as you can
see, this driver might still work on other non-RCAR platforms.

What's your take on this ?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
>> > > And, kernel log is the following:
>> > >
>> > > [1.565605] xhci-hcd ee00.usb: xHCI Host Controller
>> > > [1.570636] xhci-hcd ee00.usb: new USB bus registered, assigned 
>> > > bus number 5
>> > > [   22.270160] xhci-hcd ee00.usb: can't setup: -110
>> > > [   22.274931] xhci-hcd ee00.usb: USB bus 5 deregistered
>> > > [   22.280158] xhci-hcd: probe of ee00.usb failed with error -110
>> > >
>> > > The timestamp is strange to me. But, logs of R-Car H3 (ES1.0) and
>> > > R-Car H2 were the same.
>> >
>> > yeah, seems like your system timer is counting twice for each tick.
>> 
>> Yes, I will investigate this later.
>> 
>> > > Should I revise the commit log in detail?
>> >
>> > Sure, but let's first why this is the case. It's unclear, to me at
>> > least, why reset fails.
>> 
>> I understood it. I will investigate why reset fails first.
>
> According to the HW team of R-Car SoCs, the firmware of R-Car USB 3.0 host 
> controller
> will control the reset. So, if the xhci-rcar driver doesn't do firmware 
> downloading,
> the reset of USB3.0 host controller doesn't work correctly.
> The HW team intends to describe this specification on next datasheet revision.
> (In other words, the current datasheet doesn't mention exactly about this.)

that explains it, thanks :-)

> So, I will revise the commit log and submit such a patch later.

great, thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Hi,

Geert Uytterhoeven  writes:
> On Thu, Apr 21, 2016 at 12:27 PM, Yoshihiro Shimoda
>  wrote:
>>> > [1.565605] xhci-hcd ee00.usb: xHCI Host Controller
>>> > [1.570636] xhci-hcd ee00.usb: new USB bus registered, assigned 
>>> > bus number 5
>>> > [   22.270160] xhci-hcd ee00.usb: can't setup: -110
>>> > [   22.274931] xhci-hcd ee00.usb: USB bus 5 deregistered
>>> > [   22.280158] xhci-hcd: probe of ee00.usb failed with error -110
>>> >
>>> > The timestamp is strange to me. But, logs of R-Car H3 (ES1.0) and
>>> > R-Car H2 were the same.
>>>
>>> yeah, seems like your system timer is counting twice for each tick.
>>
>> Yes, I will investigate this later.
>
> The main clock crystal on Salvator-X is half of the expected value. But
> despite the correct value being in the DTS, there's some timer code that
> doesn't take this into account.

cool, thanks for the note. One problem down.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:

> Hi Felipe,
>
>> From: Felipe Balbi
>> Sent: Thursday, April 21, 2016 7:05 PM
>> 
>> Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
>> 
>> > If kernel configuration is CONFIG_USB_XHCI_PLATFORM=y and
>> > CONFIG_USB_XHCI_RCAR is not set, R-Car Gen2/3 will cause long wait
>> > in xhci_reset() because such SoCs need specific initialization.
>> 
>> where is the delay coming from exactly ?
>
> The delay is coming from the following code:
>
>   ret = xhci_handshake(>op_regs->command,
>   CMD_RESET, 0, 10 * 1000 * 1000);
>   if (ret)
>   return ret;

okay, and why does reset fail ?

> And, kernel log is the following:
>
> [1.565605] xhci-hcd ee00.usb: xHCI Host Controller
> [1.570636] xhci-hcd ee00.usb: new USB bus registered, assigned bus 
> number 5
> [   22.270160] xhci-hcd ee00.usb: can't setup: -110
> [   22.274931] xhci-hcd ee00.usb: USB bus 5 deregistered
> [   22.280158] xhci-hcd: probe of ee00.usb failed with error -110
>
> The timestamp is strange to me. But, logs of R-Car H3 (ES1.0) and
> R-Car H2 were the same.

yeah, seems like your system timer is counting twice for each tick.

> Should I revise the commit log in detail?

Sure, but let's first why this is the case. It's unclear, to me at
least, why reset fails.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: host: xhci-rcar: Avoid long wait in xhci_reset()

[Felipe Balbi]

Yoshihiro Shimoda  writes:

> If kernel configuration is CONFIG_USB_XHCI_PLATFORM=y and
> CONFIG_USB_XHCI_RCAR is not set, R-Car Gen2/3 will cause long wait
> in xhci_reset() because such SoCs need specific initialization.

where is the delay coming from exactly ?

> So, this patch modifies the xhci_rcar_init_quirk() in xhci-rcar.h
> to exit the probe function immediately.
>
> Fixes: 4ac8918f3a7 (usb: host: xhci-plat: add support for the R-Car H2 and M2 
> xHCI controllers)
> Cc:  # v3.17+
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  drivers/usb/host/xhci-rcar.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/xhci-rcar.h b/drivers/usb/host/xhci-rcar.h
> index 2941a25..2afed68 100644
> --- a/drivers/usb/host/xhci-rcar.h
> +++ b/drivers/usb/host/xhci-rcar.h
> @@ -24,7 +24,11 @@ static inline void xhci_rcar_start(struct usb_hcd *hcd)
>  
>  static inline int xhci_rcar_init_quirk(struct usb_hcd *hcd)
>  {
> - return 0;
> + /*
> +  * To avoid wait and timeout in xhci_reset() if CONFIG_XHCI_RCAR is
> +  * disabled, this function fails.
> +  */
> + return -ENODEV;
>  }
>  #endif
>  #endif /* _XHCI_RCAR_H */
> -- 
> 1.9.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

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 0/2] usb: host: xhci: add a new quirk and fix an issue for R-Car Gen2/3

[Felipe Balbi]

Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com> writes:
> [ text/plain ]
> This patch set is based on the latest usb.git / usb-next branch.
> (commit id = ce53bfc4374cada8b645765e2b4ad5831e760932)
>
> Changes from v2:
>  - Change the quirk name to "XHCI_NO_64BIT_SUPPORT".
>  - Add comments for R-Car Gen2/3 (like a patch v1) in xhci-plat.c.
>
> Changes from v1:
>  - Add a new quirk "XHCI_CLEAR_AC64" for using it on some PCIe card.
>http://thread.gmane.org/gmane.linux.kernel.renesas-soc/858/focus=1694
>
> Yoshihiro Shimoda (2):
>   usb: host: xhci: add a new quirk XHCI_NO_64BIT_SUPPORT
>   usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB
> phys

for your entire series:

Reviewed-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB phys

[Felipe Balbi]

Yoshihiro Shimoda  writes:

> [ text/plain ]
> This patch fixes an issue that cannot work if R-Car Gen2/3 run on
> above 4GB physical memory environment to use a quirk XHCI_CLEAR_AC64.
>
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  drivers/usb/host/xhci-plat.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 5c15e9b..4dbd56f 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -39,12 +39,19 @@ static const struct xhci_driver_overrides 
> xhci_plat_overrides __initconst = {
>  
>  static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>  {
> + struct usb_hcd *hcd = xhci_to_hcd(xhci);
> +
>   /*
>* As of now platform drivers don't provide MSI support so we ensure
>* here that the generic code does not try to make a pci_dev from our
>* dev struct in order to setup MSI
>*/
>   xhci->quirks |= XHCI_PLAT;
> +
> + /* Please refer to the xhci.c about the detail of this quirk */

this is an odd comment. It should be okay to state here that RCAR
GEN2/GEN3 lack proper 64BIT support even though register says otherwise.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB phys

[Felipe Balbi]

Hi,

Yoshihiro Shimoda  writes:
> [ text/plain ]
> Hi Mathias,
>
> Would you review this patch?
> This patch could be applied into the latest usb.git / usb-next branch.
>
> Best regards,
> Yoshihiro Shimoda
>
>> -Original Message-
>> From: Yoshihiro Shimoda
>> Sent: Tuesday, February 16, 2016 12:26 PM
>> To: mathias.ny...@intel.com; gre...@linuxfoundation.org
>> Cc: linux-...@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Yoshihiro 
>> Shimoda 
>> Subject: [PATCH] usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run 
>> on above 4GB phys
>> 
>> On xHCI controllers of R-Car Gen2 and Gen3, the AC64 bit (bit 0) of
>> HCCPARAMS1 is set to 1. However, these SoCs don't support 64-bit address
>> memory pointers. So, this driver should call dma_set_coherent_mask(dev,
>> DMA_BIT_MASK(32)) in xhci_gen_setup(). Otherwise, the xHCI controller
>> will be died after a usb device is connected if R-Car Gen2/3 run on above
>> 4GB physical memory environment.
>> 
>> This patch adds clearing the AC64 bit of xhci->hcc_params in the
>> xhci_plat_quirks() to fix the issue.
>> 
>> Signed-off-by: Yoshihiro Shimoda 
>> ---
>>  drivers/usb/host/xhci-plat.c | 13 +
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index d39d6bf..047fb6a 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -39,12 +39,25 @@ static const struct xhci_driver_overrides 
>> xhci_plat_overrides __initconst = {
>> 
>>  static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
>>  {
>> +struct usb_hcd *hcd = xhci_to_hcd(xhci);
>> +
>>  /*
>>   * As of now platform drivers don't provide MSI support so we ensure
>>   * here that the generic code does not try to make a pci_dev from our
>>   * dev struct in order to setup MSI
>>   */
>>  xhci->quirks |= XHCI_PLAT;
>> +
>> +/*
>> + * On R-Car Gen2 and Gen3, the AC64 bit (bit 0) of HCCPARAMS1 is set
>> + * to 1. However, these SoCs don't support 64-bit address memory
>> + * pointers. So, this driver clears the AC64 bit of xhci->hcc_params
>> + * to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in
>> + * xhci_gen_setup().
>> + */
>> +if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) ||
>> +xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3))
>> +xhci->hcc_params &= ~BIT(0);/* clear HCC_64BIT_ADDR
>>  */

Mathias, of course, has the final saying; but I feel that, just like any
other quirk, this should be setting a flag in xhci->quirks and xhci core
would clear hcc_params. The reason is that if we ever need this on some
PCIe card, we won't have to shuffle code around.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: renesas_usbhs: gadget: fix giveback status code in usbhsg_pipe_disable()

[Felipe Balbi]

Hi Yoshihiro,

Yoshihiro Shimoda  writes:
> [ text/plain ]
> Hi Felipe,
>
> Oops, I completely forgot this patch.
> Would you review this patch? Or should I resend it?
> I confirmed that this patch could be applied on your latest
> testing/fixes branch.

I don't seem to have the original patch in my inbox. Would you mind
resending ?

Thanks

-- 
balbi


signature.asc
Description: PGP signature