| Message ID | 1435832793-9999-1-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Phil-san, > Sent: Thursday, July 02, 2015 7:27 PM > > These changes allow a PHY driver to trigger a VBUS interrupt and > to provide the value of VBUS. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> Thank you for the patch! However, if I tested this patch, a kernel panic happened when I did "rmmod g_mass_storage" on koelsch: root@192:~/usb# rmmod g_mass_storage Unable to handle kernel NULL pointer dereference at virtual address 00000004 pgd = eebd4bc0 [00000004] *pgd=00000000 Internal error: Oops: 205 [#1] SMP ARM Modules linked in: g_mass_storage(-) usb_f_mass_storage libcomposite CPU: 0 PID: 2496 Comm: rmmod Not tainted 4.1.0-dirty #32 Hardware name: Generic R8A7791 (Flattened Device Tree) task: eeb64ac0 ti: ee022000 task.ti: ee022000 PC is at usbhs_pkt_pop+0x1c/0x100 LR is at usbhsg_ep_dequeue+0x28/0x40 pc : [<c036d934>] lr : [<c036e364>] psr: 20000013 sp : ee023e70 ip : ee023e98 fp : ee023e94 r10: 00000000 r9 : ee022000 r8 : c00101e4 r7 : 00200200 r6 : 00100100 r5 : 00000000 r4 : eeb917f8 r3 : c036e33c r2 : 00000041 r1 : eeb917f8 r0 : 00000000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user Control: 30c5307d Table: 6ebd4bc0 DAC: fffffffd Process rmmod (pid: 2496, stack limit = 0xee022210) Stack: (0xee023e70 to 0xee024000) 3e60: ee1b1400 eeb917c0 00100100 00200200 3e80: c00101e4 ee022000 ee023eac ee023e98 c036e364 c036d924 eeb34180 eeb341c0 3ea0: ee023ecc ee023eb0 bf002f20 c036e348 eeb34180 00000001 ee1b1800 eeb341b8 3ec0: ee023eec ee023ed0 bf002fec bf002e5c ee181600 bf01d624 bed3ff0d 00000081 3ee0: ee023efc ee023ef0 bf003028 bf002f78 ee023f14 ee023f00 c036fb88 bf003018 3f00: ee181600 bf01d624 ee023f2c ee023f18 c036fc10 c036fb34 bf01d694 7f64cd30 3f20: ee023f3c ee023f30 bf0013d0 c036fbbc ee023f4c ee023f40 bf01d26c bf0013c0 3f40: ee023fa4 ee023f50 c0090bd8 bf01d248 ee023f6c 616d5f67 735f7373 61726f74 3f60: 00006567 ee023f70 c00460f4 c052a8c0 c00101e4 ee022010 ee023fb0 ee022000 3f80: ee023fac ee023f90 c00136c4 00046038 00000001 7f64cd00 00000000 ee023fa8 3fa0: c0010040 c0090adc 00000001 7f64cd00 7f64cd30 00000800 ccd5c100 ccd5c100 3fc0: 00000001 7f64cd00 bed3ff0d 00000081 7f64c008 00000000 00000002 00000800 3fe0: bed3fe0c bed3fb98 b6eeb068 b6e4f05c 60000010 7f64cd30 00000000 00000000 Backtrace: [<c036d918>] (usbhs_pkt_pop) from [<c036e364>] (usbhsg_ep_dequeue+0x28/0x40) r9:ee022000 r8:c00101e4 r7:00200200 r6:00100100 r5:eeb917c0 r4:ee1b1400 [<c036e33c>] (usbhsg_ep_dequeue) from [<bf002f20>] (composite_dev_cleanup+0xd0/0x11c [libcomposite]) r5:eeb341c0 r4:eeb34180 [<bf002e50>] (composite_dev_cleanup [libcomposite]) from [<bf002fec>] (__composite_unbind+0x80/0xa0 [libcomposite]) r7:eeb341b8 r6:ee1b1800 r5:00000001 r4:eeb34180 [<bf002f6c>] (__composite_unbind [libcomposite]) from [<bf003028>] (composite_unbind+0x1c/0x20 [libcomposite]) r7:00000081 r6:bed3ff0d r5:bf01d624 r4:ee181600 [<bf00300c>] (composite_unbind [libcomposite]) from [<c036fb88>] (usb_gadget_remove_driver+0x60/0x88) [<c036fb28>] (usb_gadget_remove_driver) from [<c036fc10>] (usb_gadget_unregister_driver+0x60/0xa0) r5:bf01d624 r4:ee181600 [<c036fbb0>] (usb_gadget_unregister_driver) from [<bf0013d0>] (usb_composite_unregister+0x1c/0x20 [libcomposite]) r5:7f64cd30 r4:bf01d694 [<bf0013b4>] (usb_composite_unregister [libcomposite]) from [<bf01d26c>] (msg_cleanup+0x30/0x3c [g_mass_storage]) [<bf01d23c>] (msg_cleanup [g_mass_storage]) from [<c0090bd8>] (SyS_delete_module+0x108/0x1c4) [<c0090ad0>] (SyS_delete_module) from [<c0010040>] (ret_fast_syscall+0x0/0x3c) r5:7f64cd00 r4:00000001 Code: e52de004 e8bd4000 e1a05000 e1a04001 (e5907004) ---[ end trace 8152e492b3f02f66 ]--- I don't know why this kernel panic happened after I applied this patch. But, if I added the following code in the mod_gadget.c, this issue disappeared: @@ -682,7 +684,8 @@ static int usbhsg_ep_dequeue(struct usb_ep *ep, struct usb_r struct usbhsg_request *ureq = usbhsg_req_to_ureq(req); struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep); - usbhs_pkt_pop(pipe, usbhsg_ureq_to_pkt(ureq)); + if (pipe) + usbhs_pkt_pop(pipe, usbhsg_ureq_to_pkt(ureq)); usbhsg_queue_pop(uep, ureq, -ECONNRESET); return 0; Best regards, Yoshihiro Shimoda -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Phil, Thank you for the patch. On Thursday 02 July 2015 11:26:33 Phil Edworthy wrote: > These changes allow a PHY driver to trigger a VBUS interrupt and > to provide the value of VBUS. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > v3: > - Changed how indirect vbus is plumbed in. > - Removed unnecessary (void) on call to otg_set_peripheral. > - Moved code that connects to bus through transceiver so it > is before setting gpriv->driver. > > v2: > - vbus variables changed from int to bool. > - dev_info() changed to dev_err() > --- > drivers/usb/renesas_usbhs/common.c | 8 +++++-- > drivers/usb/renesas_usbhs/common.h | 2 ++ > drivers/usb/renesas_usbhs/mod_gadget.c | 38 +++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/renesas_usbhs/common.c > b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644 > --- a/drivers/usb/renesas_usbhs/common.c > +++ b/drivers/usb/renesas_usbhs/common.c > @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv) > /* > * get vbus status from platform > */ > - enable = usbhs_platform_call(priv, get_vbus, pdev); > + if (priv->vbus_is_indirect) > + enable = priv->vbus_indirect_value; > + else > + enable = usbhs_platform_call(priv, get_vbus, pdev); > > /* > * get id from platform > @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev) > pm_runtime_enable(&pdev->dev); > if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) { > usbhsc_power_ctrl(priv, 1); > - usbhs_mod_autonomy_mode(priv); > + if (!priv->vbus_is_indirect) Isn't this racy ? The vbus_session operation is called by transceivers from interrupt handlers or work queues. Do we have a guarantee that it will be called at least once before this code is reached ? Please see below for a possible way to fix this. > + usbhs_mod_autonomy_mode(priv); > } > > /* > diff --git a/drivers/usb/renesas_usbhs/common.h > b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644 > --- a/drivers/usb/renesas_usbhs/common.h > +++ b/drivers/usb/renesas_usbhs/common.h > @@ -255,6 +255,8 @@ struct usbhs_priv { > struct renesas_usbhs_driver_param dparam; > > struct delayed_work notify_hotplug_work; > + bool vbus_is_indirect; > + bool vbus_indirect_value; > struct platform_device *pdev; > > struct extcon_dev *edev; > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644 > --- a/drivers/usb/renesas_usbhs/mod_gadget.c > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c > @@ -21,6 +21,7 @@ > #include <linux/platform_device.h> > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > +#include <linux/usb/otg.h> > #include "common.h" > > /* > @@ -50,6 +51,7 @@ struct usbhsg_gpriv { > int uep_size; > > struct usb_gadget_driver *driver; > + struct usb_phy *transceiver; > > u32 status; > #define USBHSG_STATUS_STARTED (1 << 0) > @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget > *gadget, { > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > + struct device *dev = usbhs_priv_to_dev(priv); > + int ret; > > if (!driver || > !driver->setup || > driver->max_speed < USB_SPEED_FULL) > return -EINVAL; > > + /* connect to bus through transceiver */ > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) { > + ret = otg_set_peripheral(gpriv->transceiver->otg, > + &gpriv->gadget); > + if (ret) { > + dev_err(dev, "%s: can't bind to transceiver\n", > + gpriv->gadget.name); > + return ret; > + } > + } > + How is this change related to the topic ? I'm not too familiar with this driver so I might just have missed something, but then others could as well, and the commit message could do with a bit more information. > /* first hook up the driver ... */ > gpriv->driver = driver; > > @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD); > + > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) > + otg_set_peripheral(gpriv->transceiver->otg, NULL); > + > gpriv->driver = NULL; > > return 0; > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget > *gadget, int is_self) return 0; > } > > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active) > +{ > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > + struct platform_device *pdev = usbhs_priv_to_pdev(priv); > + > + priv->vbus_is_indirect = 1; vbus_is_indirect is never reset to 0. Shouldn't you set it to 1 (or rather to true) in usbhs_mod_gadget_probe() if an external transceiver is found ? What would you think of implementing and wiring up the get_vbus operation in usbhs_mod_gadget_probe() in the same way that usbhs_mod_autonomy_mode() does, and return the value of vbus_indirect_value there ? You could then get rid of the vbus_is_indirect field and move the vbus_indirect_value field to struct usbhsg_gpriv, removing the need to modify drivers/usb/renesas_usbhs/common.c. It should also solve the race condition mentioned above. > + priv->vbus_indirect_value = !!is_active; > + > + renesas_usbhs_call_notify_hotplug(pdev); > + > + return 0; > +} > + > static const struct usb_gadget_ops usbhsg_gadget_ops = { > .get_frame = usbhsg_get_frame, > .set_selfpowered = usbhsg_set_selfpowered, > .udc_start = usbhsg_gadget_start, > .udc_stop = usbhsg_gadget_stop, > .pullup = usbhsg_pullup, > + .vbus_session = usbhsg_vbus_session, > }; > > static int usbhsg_start(struct usbhs_priv *priv) > @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv) > goto usbhs_mod_gadget_probe_err_gpriv; > } > > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED); > + dev_info(dev, "%s transceiver found\n", > + gpriv->transceiver ? "" : "No"); > + > /* > * CAUTION > *
Hi Shimoda-san, On 06 July 2015 08:28, Shimoda-san wrote: > Hi Phil-san, > > > Sent: Thursday, July 02, 2015 7:27 PM > > > > These changes allow a PHY driver to trigger a VBUS interrupt and > > to provide the value of VBUS. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > Thank you for the patch! > > However, if I tested this patch, a kernel panic happened when > I did "rmmod g_mass_storage" on koelsch: > > root@192:~/usb# rmmod g_mass_storage > Unable to handle kernel NULL pointer dereference at virtual address 00000004 > pgd = eebd4bc0 > [00000004] *pgd=00000000 > Internal error: Oops: 205 [#1] SMP ARM > Modules linked in: g_mass_storage(-) usb_f_mass_storage libcomposite > CPU: 0 PID: 2496 Comm: rmmod Not tainted 4.1.0-dirty #32 > Hardware name: Generic R8A7791 (Flattened Device Tree) > task: eeb64ac0 ti: ee022000 task.ti: ee022000 > PC is at usbhs_pkt_pop+0x1c/0x100 > LR is at usbhsg_ep_dequeue+0x28/0x40 > pc : [<c036d934>] lr : [<c036e364>] psr: 20000013 > sp : ee023e70 ip : ee023e98 fp : ee023e94 > r10: 00000000 r9 : ee022000 r8 : c00101e4 > r7 : 00200200 r6 : 00100100 r5 : 00000000 r4 : eeb917f8 > r3 : c036e33c r2 : 00000041 r1 : eeb917f8 r0 : 00000000 > Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > Control: 30c5307d Table: 6ebd4bc0 DAC: fffffffd > Process rmmod (pid: 2496, stack limit = 0xee022210) > Stack: (0xee023e70 to 0xee024000) > 3e60: ee1b1400 eeb917c0 00100100 00200200 > 3e80: c00101e4 ee022000 ee023eac ee023e98 c036e364 c036d924 eeb34180 > eeb341c0 > 3ea0: ee023ecc ee023eb0 bf002f20 c036e348 eeb34180 00000001 ee1b1800 > eeb341b8 > 3ec0: ee023eec ee023ed0 bf002fec bf002e5c ee181600 bf01d624 bed3ff0d > 00000081 > 3ee0: ee023efc ee023ef0 bf003028 bf002f78 ee023f14 ee023f00 c036fb88 > bf003018 > 3f00: ee181600 bf01d624 ee023f2c ee023f18 c036fc10 c036fb34 bf01d694 7f64cd30 > 3f20: ee023f3c ee023f30 bf0013d0 c036fbbc ee023f4c ee023f40 bf01d26c bf0013c0 > 3f40: ee023fa4 ee023f50 c0090bd8 bf01d248 ee023f6c 616d5f67 735f7373 > 61726f74 > 3f60: 00006567 ee023f70 c00460f4 c052a8c0 c00101e4 ee022010 ee023fb0 > ee022000 > 3f80: ee023fac ee023f90 c00136c4 00046038 00000001 7f64cd00 00000000 > ee023fa8 > 3fa0: c0010040 c0090adc 00000001 7f64cd00 7f64cd30 00000800 ccd5c100 ccd5c100 > 3fc0: 00000001 7f64cd00 bed3ff0d 00000081 7f64c008 00000000 00000002 00000800 > 3fe0: bed3fe0c bed3fb98 b6eeb068 b6e4f05c 60000010 7f64cd30 00000000 > 00000000 > Backtrace: > [<c036d918>] (usbhs_pkt_pop) from [<c036e364>] > (usbhsg_ep_dequeue+0x28/0x40) > r9:ee022000 r8:c00101e4 r7:00200200 r6:00100100 r5:eeb917c0 r4:ee1b1400 > [<c036e33c>] (usbhsg_ep_dequeue) from [<bf002f20>] > (composite_dev_cleanup+0xd0/0x11c [libcomposite]) > r5:eeb341c0 r4:eeb34180 > [<bf002e50>] (composite_dev_cleanup [libcomposite]) from [<bf002fec>] > (__composite_unbind+0x80/0xa0 [libcomposite]) > r7:eeb341b8 r6:ee1b1800 r5:00000001 r4:eeb34180 > [<bf002f6c>] (__composite_unbind [libcomposite]) from [<bf003028>] > (composite_unbind+0x1c/0x20 [libcomposite]) > r7:00000081 r6:bed3ff0d r5:bf01d624 r4:ee181600 > [<bf00300c>] (composite_unbind [libcomposite]) from [<c036fb88>] > (usb_gadget_remove_driver+0x60/0x88) > [<c036fb28>] (usb_gadget_remove_driver) from [<c036fc10>] > (usb_gadget_unregister_driver+0x60/0xa0) > r5:bf01d624 r4:ee181600 > [<c036fbb0>] (usb_gadget_unregister_driver) from [<bf0013d0>] > (usb_composite_unregister+0x1c/0x20 [libcomposite]) > r5:7f64cd30 r4:bf01d694 > [<bf0013b4>] (usb_composite_unregister [libcomposite]) from [<bf01d26c>] > (msg_cleanup+0x30/0x3c [g_mass_storage]) > [<bf01d23c>] (msg_cleanup [g_mass_storage]) from [<c0090bd8>] > (SyS_delete_module+0x108/0x1c4) > [<c0090ad0>] (SyS_delete_module) from [<c0010040>] > (ret_fast_syscall+0x0/0x3c) > r5:7f64cd00 r4:00000001 > Code: e52de004 e8bd4000 e1a05000 e1a04001 (e5907004) > ---[ end trace 8152e492b3f02f66 ]--- > > > I don't know why this kernel panic happened after I applied this patch. > But, if I added the following code in the mod_gadget.c, this issue disappeared: > > @@ -682,7 +684,8 @@ static int usbhsg_ep_dequeue(struct usb_ep *ep, struct > usb_r > struct usbhsg_request *ureq = usbhsg_req_to_ureq(req); > struct usbhs_pipe *pipe = usbhsg_uep_to_pipe(uep); > > - usbhs_pkt_pop(pipe, usbhsg_ureq_to_pkt(ureq)); > + if (pipe) > + usbhs_pkt_pop(pipe, usbhsg_ureq_to_pkt(ureq)); > usbhsg_queue_pop(uep, ureq, -ECONNRESET); > > return 0; That's odd, I can't see how my patch causes this problem. Do you think that there has always been a race problem here and my changes make this happen? > Best regards, > Yoshihiro Shimoda > Best regards Phil -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On 06 July 2015 09:20, Laurent wrote: > Hi Phil, > > Thank you for the patch. Thanks for your review! > On Thursday 02 July 2015 11:26:33 Phil Edworthy wrote: > > These changes allow a PHY driver to trigger a VBUS interrupt and > > to provide the value of VBUS. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > --- > > v3: > > - Changed how indirect vbus is plumbed in. > > - Removed unnecessary (void) on call to otg_set_peripheral. > > - Moved code that connects to bus through transceiver so it > > is before setting gpriv->driver. > > > > v2: > > - vbus variables changed from int to bool. > > - dev_info() changed to dev_err() > > --- > > drivers/usb/renesas_usbhs/common.c | 8 +++++-- > > drivers/usb/renesas_usbhs/common.h | 2 ++ > > drivers/usb/renesas_usbhs/mod_gadget.c | 38 > +++++++++++++++++++++++++++++++ > > 3 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/renesas_usbhs/common.c > > b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644 > > --- a/drivers/usb/renesas_usbhs/common.c > > +++ b/drivers/usb/renesas_usbhs/common.c > > @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv) > > /* > > * get vbus status from platform > > */ > > - enable = usbhs_platform_call(priv, get_vbus, pdev); > > + if (priv->vbus_is_indirect) > > + enable = priv->vbus_indirect_value; > > + else > > + enable = usbhs_platform_call(priv, get_vbus, pdev); > > > > /* > > * get id from platform > > @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev) > > pm_runtime_enable(&pdev->dev); > > if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) { > > usbhsc_power_ctrl(priv, 1); > > - usbhs_mod_autonomy_mode(priv); > > + if (!priv->vbus_is_indirect) > > Isn't this racy ? The vbus_session operation is called by transceivers from > interrupt handlers or work queues. Do we have a guarantee that it will be > called at least once before this code is reached ? Please see below for a > possible way to fix this. You are right, it is racy... :( > > + usbhs_mod_autonomy_mode(priv); > > } > > > > /* > > diff --git a/drivers/usb/renesas_usbhs/common.h > > b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644 > > --- a/drivers/usb/renesas_usbhs/common.h > > +++ b/drivers/usb/renesas_usbhs/common.h > > @@ -255,6 +255,8 @@ struct usbhs_priv { > > struct renesas_usbhs_driver_param dparam; > > > > struct delayed_work notify_hotplug_work; > > + bool vbus_is_indirect; > > + bool vbus_indirect_value; > > struct platform_device *pdev; > > > > struct extcon_dev *edev; > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c > > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644 > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c > > @@ -21,6 +21,7 @@ > > #include <linux/platform_device.h> > > #include <linux/usb/ch9.h> > > #include <linux/usb/gadget.h> > > +#include <linux/usb/otg.h> > > #include "common.h" > > > > /* > > @@ -50,6 +51,7 @@ struct usbhsg_gpriv { > > int uep_size; > > > > struct usb_gadget_driver *driver; > > + struct usb_phy *transceiver; > > > > u32 status; > > #define USBHSG_STATUS_STARTED (1 << 0) > > @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget > > *gadget, { > > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); > > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > > + struct device *dev = usbhs_priv_to_dev(priv); > > + int ret; > > > > if (!driver || > > !driver->setup || > > driver->max_speed < USB_SPEED_FULL) > > return -EINVAL; > > > > + /* connect to bus through transceiver */ > > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) { > > + ret = otg_set_peripheral(gpriv->transceiver->otg, > > + &gpriv->gadget); > > + if (ret) { > > + dev_err(dev, "%s: can't bind to transceiver\n", > > + gpriv->gadget.name); > > + return ret; > > + } > > + } > > + > > How is this change related to the topic ? I'm not too familiar with this > driver so I might just have missed something, but then others could as well, > and the commit message could do with a bit more information. I'm not sure what you mean, this code is needed to tell the phy driver if it is in host or gadget mode. I would have thought this is clear as it calls otg_set_peripheral. > > /* first hook up the driver ... */ > > gpriv->driver = driver; > > > > @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget > > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > > > > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD); > > + > > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) > > + otg_set_peripheral(gpriv->transceiver->otg, NULL); > > + > > gpriv->driver = NULL; > > > > return 0; > > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget > > *gadget, int is_self) return 0; > > } > > > > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active) > > +{ > > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); > > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); > > + struct platform_device *pdev = usbhs_priv_to_pdev(priv); > > + > > + priv->vbus_is_indirect = 1; > > vbus_is_indirect is never reset to 0. Shouldn't you set it to 1 (or rather to > true) in usbhs_mod_gadget_probe() if an external transceiver is found ? Yes, I see what you say. > What would you think of implementing and wiring up the get_vbus operation in > usbhs_mod_gadget_probe() in the same way that > usbhs_mod_autonomy_mode() does, > and return the value of vbus_indirect_value there ? You could then get rid of > the vbus_is_indirect field and move the vbus_indirect_value field to struct > usbhsg_gpriv, removing the need to modify > drivers/usb/renesas_usbhs/common.c. > It should also solve the race condition mentioned above. Ok, I'll have a look at that. > > + priv->vbus_indirect_value = !!is_active; > > + > > + renesas_usbhs_call_notify_hotplug(pdev); > > + > > + return 0; > > +} > > + > > static const struct usb_gadget_ops usbhsg_gadget_ops = { > > .get_frame = usbhsg_get_frame, > > .set_selfpowered = usbhsg_set_selfpowered, > > .udc_start = usbhsg_gadget_start, > > .udc_stop = usbhsg_gadget_stop, > > .pullup = usbhsg_pullup, > > + .vbus_session = usbhsg_vbus_session, > > }; > > > > static int usbhsg_start(struct usbhs_priv *priv) > > @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv > *priv) > > goto usbhs_mod_gadget_probe_err_gpriv; > > } > > > > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED); > > + dev_info(dev, "%s transceiver found\n", > > + gpriv->transceiver ? "" : "No"); > > + > > /* > > * CAUTION > > * > > -- > Regards, > > Laurent Pinchart Thanks Phil -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv) /* * get vbus status from platform */ - enable = usbhs_platform_call(priv, get_vbus, pdev); + if (priv->vbus_is_indirect) + enable = priv->vbus_indirect_value; + else + enable = usbhs_platform_call(priv, get_vbus, pdev); /* * get id from platform @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev) pm_runtime_enable(&pdev->dev); if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) { usbhsc_power_ctrl(priv, 1); - usbhs_mod_autonomy_mode(priv); + if (!priv->vbus_is_indirect) + usbhs_mod_autonomy_mode(priv); } /* diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644 --- a/drivers/usb/renesas_usbhs/common.h +++ b/drivers/usb/renesas_usbhs/common.h @@ -255,6 +255,8 @@ struct usbhs_priv { struct renesas_usbhs_driver_param dparam; struct delayed_work notify_hotplug_work; + bool vbus_is_indirect; + bool vbus_indirect_value; struct platform_device *pdev; struct extcon_dev *edev; diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644 --- a/drivers/usb/renesas_usbhs/mod_gadget.c +++ b/drivers/usb/renesas_usbhs/mod_gadget.c @@ -21,6 +21,7 @@ #include <linux/platform_device.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> +#include <linux/usb/otg.h> #include "common.h" /* @@ -50,6 +51,7 @@ struct usbhsg_gpriv { int uep_size; struct usb_gadget_driver *driver; + struct usb_phy *transceiver; u32 status; #define USBHSG_STATUS_STARTED (1 << 0) @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget, { struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); + struct device *dev = usbhs_priv_to_dev(priv); + int ret; if (!driver || !driver->setup || driver->max_speed < USB_SPEED_FULL) return -EINVAL; + /* connect to bus through transceiver */ + if (!IS_ERR_OR_NULL(gpriv->transceiver)) { + ret = otg_set_peripheral(gpriv->transceiver->otg, + &gpriv->gadget); + if (ret) { + dev_err(dev, "%s: can't bind to transceiver\n", + gpriv->gadget.name); + return ret; + } + } + /* first hook up the driver ... */ gpriv->driver = driver; @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD); + + if (!IS_ERR_OR_NULL(gpriv->transceiver)) + otg_set_peripheral(gpriv->transceiver->otg, NULL); + gpriv->driver = NULL; return 0; @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget *gadget, int is_self) return 0; } +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active) +{ + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget); + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv); + struct platform_device *pdev = usbhs_priv_to_pdev(priv); + + priv->vbus_is_indirect = 1; + priv->vbus_indirect_value = !!is_active; + + renesas_usbhs_call_notify_hotplug(pdev); + + return 0; +} + static const struct usb_gadget_ops usbhsg_gadget_ops = { .get_frame = usbhsg_get_frame, .set_selfpowered = usbhsg_set_selfpowered, .udc_start = usbhsg_gadget_start, .udc_stop = usbhsg_gadget_stop, .pullup = usbhsg_pullup, + .vbus_session = usbhsg_vbus_session, }; static int usbhsg_start(struct usbhs_priv *priv) @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv) goto usbhs_mod_gadget_probe_err_gpriv; } + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED); + dev_info(dev, "%s transceiver found\n", + gpriv->transceiver ? "" : "No"); + /* * CAUTION *
These changes allow a PHY driver to trigger a VBUS interrupt and to provide the value of VBUS. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- v3: - Changed how indirect vbus is plumbed in. - Removed unnecessary (void) on call to otg_set_peripheral. - Moved code that connects to bus through transceiver so it is before setting gpriv->driver. v2: - vbus variables changed from int to bool. - dev_info() changed to dev_err() --- drivers/usb/renesas_usbhs/common.c | 8 +++++-- drivers/usb/renesas_usbhs/common.h | 2 ++ drivers/usb/renesas_usbhs/mod_gadget.c | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-)