Re: MUSB peripheral DMA regression caused by driver core runtime PM change
On Fri, 23 Oct 2015, Grygorii Strashko wrote:
> Reviewed-by: Grygorii Strashko <grygorii.stras...@ti.com>
>
> It always fun when DD/PM core is updated to fix some driver/subsystem's
> specific PM issue :(
>
> >
> >> --- a/drivers/usb/musb/omap2430.c
> >> +++ b/drivers/usb/musb/omap2430.c
> >> @@ -391,9 +391,20 @@ static int omap2430_musb_init(struct musb *musb)
> >>}
> >>musb->isr = omap2430_musb_interrupt;
> >>
> >> + /*
> >> + * Enable runtime PM for musb parent (this driver). We can't
> >> + * do it earlier as struct musb is not yet allocated and we
> >> + * need to touch the musb registers for runtime PM.
> >> + */
> >> + pm_runtime_enable(glue->dev);
> >> + status = pm_runtime_get_sync(glue->dev);
> >> + if (status < 0)
> >> + goto err1;
> >> +
> >>status = pm_runtime_get_sync(dev);
>
> Hm. My assumption was that *Parent* device (omap2430) will be enabled
> here :( But as I can see this will not happen:
>
> static int rpm_resume(struct device *dev, int rpmflags)
> {...
> if (!parent && dev->parent) {
> /*
>* Increment the parent's usage counter and resume it if
>* necessary. Not needed if dev is irq-safe; then the
>* parent is permanently resumed.
>*/
> parent = dev->parent;
> if (dev->power.irq_safe)
> goto skip_parent;
>
> ^^^ and musb device is irq_safe :(
This way of doing things looks very strange.
If the omap2430 is the parent of the musb device, and
pm_runtime_irq_safe() has been called for the musb device, then after
that the omap2430 will never be runtime suspended. Therefore it
doesn't matter whether you enable it for runtime PM or not.
It seems to me that the real problem must be that the musb device gets
runtime-enabled and marked irq_safe too early. These things should
happen before the musb device gets registered and exposed to userspace,
but not before the omap2430 parent is runtime-enabled.
Thus the sequence of events should be:
Allocate the musb device;
Runtime-enable the omap2430 (since it is now safe to do so);
Runtime-enable the musb and declare it irq_safe (this will
automatically runtime-resume the omap2430);
Register the musb.
If things are done this way, no special action needs to be taken.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MUSB peripheral DMA regression caused by driver core runtime PM change
On Fri, 23 Oct 2015, Tony Lindgren wrote: > > Thus the sequence of events should be: > > > > Allocate the musb device; > > Runtime-enable the omap2430 (since it is now safe to do so); > > Runtime-enable the musb and declare it irq_safe (this will > > automatically runtime-resume the omap2430); > > Register the musb. > > > > If things are done this way, no special action needs to be taken. > > Yes good point, that requires changing the init for the whole > drivers/musb though. This will have to be done anyway, since the way it is now (if I understand correctly), the musb is registered and made available to userspace before its parent is operational (i.e., at full power). > Also, we should reorganize the whole musb and make > the platform glue and musb core drivers completely separate using a > shared interrupt where needed. > > For the regression for the -rc series? Do you see any better > alternatives to what I posted? No. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 07/13] usb: otg: add OTG core
On Wed, 9 Sep 2015, Roger Quadros wrote: > (adding back folks in cc) > > On 08/09/15 20:35, Alan Stern wrote: > > On Tue, 8 Sep 2015, Roger Quadros wrote: > > > >>>> What if there is another architecture like so? > >>>> > >>>> C: > >>>> [Parent] > >>>> | > >>>> | > >>>> |--|--| > >>>> [OTG core] [gadget][host] > >>>> > >>>> We need a more flexible mechanism to link the gadget and > >>>> host device to the otg core for non DT case. > >>>> > >>>> How about adding struct usb_otg parameter to usb_otg_register_hcd()? > >>>> > >>>> e.g. > >>>> int usb_otg_register_hcd(struct usb_otg *otg, struct usb_hcd *hcd, ..) > >>>> > >>>> If otg is NULL it will try DT otg-controller property or parent to > >>>> get the otg controller. > >>> > >>> This seems a lot like something Peter and I discussed recently. See > >>> > >>> http://marc.info/?l=linux-usb=143977568021328=2 > >>> > >>> and the following messages in that thread. > >>> > >> > >> If I understood right, your proposal was to add a usb_pointers data > >> struct to the device's drvdata? > > > > Right. > > > >> This is fine only if the otg/gadget/host share the same device. > >> It does not solve the problem where each have different platform devices. > > > > Why not? Each of the platform devices can store a pointer to the same > > usb_pointers structure. Wouldn't that be suitable? > > > > I didn't understand how all of them get the reference to the > same usb_pointer structure (or contents) when their respective platform > devices > get created so that they can store it in their respective drvdata. > > Worst case, the 3 devices could be totally independent of each other without a > common parent, and get registered at different times. > > The common resource here is the physical USB port for which the 3 drivers > otg/host/gadget are being registered. > There should be a mechanism for each device to tell that it is part of > this particular USB port. (or usb_pointers struct) True, it's not clear how to set up the common relationship among the three drivers. But there must be some way to do it if they all are talking to the same port or PHY. The details are an issue for the platform-specific code to handle. I'm not sure what would be involved; maybe you can invent something. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 07/13] usb: otg: add OTG core
On Tue, 8 Sep 2015, Roger Quadros wrote:
> On 08/09/15 11:31, Peter Chen wrote:
> > On Mon, Sep 07, 2015 at 01:23:01PM +0300, Roger Quadros wrote:
> >> On 07/09/15 04:23, Peter Chen wrote:
> >>> On Mon, Aug 24, 2015 at 04:21:18PM +0300, Roger Quadros wrote:
> >>>> + * This is used by the USB Host stack to register the Host controller
> >>>> + * to the OTG core. Host controller must not be started by the
> >>>> + * caller as it is left upto the OTG state machine to do so.
> >>>> + *
> >>>> + * Returns: 0 on success, error value otherwise.
> >>>> + */
> >>>> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> >>>> + unsigned long irqflags, struct otg_hcd_ops
> >>>> *ops)
> >>>> +{
> >>>> +struct usb_otg *otgd;
> >>>> +struct device *hcd_dev = hcd->self.controller;
> >>>> +struct device *otg_dev = usb_otg_get_device(hcd_dev);
> >>>> +
> >>>
> >>> One big problem here is: there are two designs for current (IP) driver
> >>> code, one creates dedicated hcd device as roothub's parent, like dwc3.
> >>> Another one doesn't do this, roothub's parent is core device (or otg
> >>> device
> >>> in your patch), like chipidea and dwc2.
> >>>
> >>> Then, otg_dev will be glue layer device for chipidea after that.
> >>
> >> OK. Let's add a way for the otg controller driver to provide the host and
> >> gadget
> >> information to the otg core for such devices like chipidea and dwc2.
> >>
> >
> > Roger, not only chipidea and dwc2, I think the musb uses the same
> > hierarchy. If the host, device, and otg share the same register
> > region, host part can't be a platform driver since we don't want
> > to remap the same register region again.
> >
> > So, in the design, we may need to consider both situations, one
> > is otg/host/device has its own register region, and host is a
> > separate platform device (A), the other is three parts share the
> > same register region, there is only one platform driver (B).
> >
> > A:
> >
> > IP core device
> > |
> > |
> > |-|-|
> > gadget host platform device
> > |
> > roothub
> >
> > B:
> >
> > IP core device
> > |
> > |
> > |-|-|
> > gadget roothub
> >
> >
> >> This API must be called before the hcd/gadget-driver is added so that the
> >> otg
> >> core knows it's linked to an OTG controller.
> >>
> >> Any better idea?
> >>
> >
> > A flag stands for this hcd controller is the same with otg controller
> > can be used, this flag can be stored at struct usb_otg_config.
>
> What if there is another architecture like so?
>
> C:
> [Parent]
> |
> |
> |--|--|
> [OTG core] [gadget][host]
>
> We need a more flexible mechanism to link the gadget and
> host device to the otg core for non DT case.
>
> How about adding struct usb_otg parameter to usb_otg_register_hcd()?
>
> e.g.
> int usb_otg_register_hcd(struct usb_otg *otg, struct usb_hcd *hcd, ..)
>
> If otg is NULL it will try DT otg-controller property or parent to
> get the otg controller.
This seems a lot like something Peter and I discussed recently. See
http://marc.info/?l=linux-usb=143977568021328=2
and the following messages in that thread.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb: dwc3: ep0: Fix mem corruption on OUT transfers of more than 512 bytes
On Tue, 9 Jun 2015, Kishon Vijay Abraham I wrote: Why not just make the bounce buffer size the same as the maxpacket size? In other words, 1024 bytes instead of 512, for ep0 on a USB-3 device. It would still be possible for the host to send data more than 1024 bytes no? Yes. When working with DFU gadget, I've seen host sends data upto 4KB. Changing the bounce buffer size might not be able to fix all the cases IMO. The actual fix will be something like [1] [1] - http://comments.gmane.org/gmane.linux.kernel/1883688 But with a bounce buffer that's only 512 bytes long, you can never send an entire packet's worth of data. If the bounce buffer is 1024 bytes then you can send the entire first packet. When that's done, you can send the second packet. And so on. It wouldn't be quite as fast, but for ep0 that shouldn't matter. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb: dwc3: ep0: Fix mem corruption on OUT transfers of more than 512 bytes
On Tue, 9 Jun 2015, Kishon Vijay Abraham I wrote: Hi, On Tuesday 09 June 2015 08:09 PM, Michael Trimarchi wrote: Hi On Jun 9, 2015 4:36 PM, Kishon Vijay Abraham I kis...@ti.com mailto:kis...@ti.com wrote: DWC3 uses bounce buffer to handle non max packet aligned OUT transfers and the size of bounce buffer is 512 bytes. However if the host initiates OUT transfers of size more than 512 bytes (and non max packet aligned), the driver throws a WARN dump but still programs the TRB to receive more than 512 bytes. This will cause bounce buffer to overflow and corrupt the adjacent memory locations which can be fatal. Fix it by programming the TRB to receive a maximum of DWC3_EP0_BOUNCE_SIZE (512) bytes. Signed-off-by: Kishon Vijay Abraham I kis...@ti.com mailto:kis...@ti.com --- Steps to see the issue (before this patch) 1) Insert g_zero in DUT 2) run './testusb -t 14 -c 1 -s 520 -v 1' in host (size should be 512) The test should FAIL since bounce buffer can handle only 512 bytes, but the test PASS. There is a WARN dump in DUT but still there will be memory corruption since the bounce buffer overflows. Tested this patch using USB3 Gen X CV (ch9 tests: usb2 and usb3, link layer testing and MSC tests) and using USB2 X CV (ch9 tests, MSC tests). After the patch, the tests timeout! ./testusb -t 14 -c 1 -s 514 -v 1 unknown speed /dev/bus/usb/001/0180 /dev/bus/usb/001/018 test 14 -- 110 (Connection timed out) IMO a patch to fix this is required for stable releases too. So If this patch is alright, I can post the patch cc'ing stable. While the actual fix would be to have chained TRB, I'm not sure if it can go to stable releases. drivers/usb/dwc3/ep0.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index 2ef3c8d..8858c60 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -816,6 +816,11 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc, unsigned maxp = ep0-endpoint.maxpacket; transfer_size += (maxp - (transfer_size % maxp)); + + /* Maximum of DWC3_EP0_BOUNCE_SIZE can only be received */ + if (transfer_size DWC3_EP0_BOUNCE_SIZE) + transfer_size = DWC3_EP0_BOUNCE_SIZE; + Can you just use maxp in the correct way? what do you mean by correct way? Using roundup() to calculate transfer_size? Why not just make the bounce buffer size the same as the maxpacket size? In other words, 1024 bytes instead of 512, for ep0 on a USB-3 device. Alan stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb: dwc3: ep0: Fix mem corruption on OUT transfers of more than 512 bytes
On Tue, 9 Jun 2015, Kishon Vijay Abraham I wrote: But with a bounce buffer that's only 512 bytes long, you can never send an entire packet's worth of data. If the bounce buffer is 1024 bytes for control endpoint, 512 bytes should be sufficient to send entire packet right? Yes, you're right. I had confused control endpoints with bulk endpoints, where the maxpacket size is 1024. Sorry for the mistake. then you can send the entire first packet. When that's done, you can send the second packet. And so on. It wouldn't be quite as fast, but for ep0 that shouldn't matter. right! this is a variant of what I tried to implement in chained TRB [1]. $subject tries just to avoid memory corruption instead of actually trying to receive all the data. Okay. If you take the $SUBJECT approach, I think it would be better for an URB submission to fail than for the host controller to send only part of the data. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH v2 06/13] usb: hcd: Add hcd add/remove functions for OTG use
On Mon, 20 Apr 2015, Roger Quadros wrote: I don't understand this. Why do you want to defer the add/remove if the device is OTG? Don't host controller drivers expect these things to execute synchronously? Sorry for the wrong information. We actually defer only the add as the OTG state machine might not yet be in Host ready mode. The remove is always synchronous and we ensure that the HCD is removed when usb_otg_unregister_hcd() returns. That's okay then, but it does leave one potential hole. What happens if usb_add_hcd() is deferred for so long that usb_remove_hcd() gets called before the add can complete? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH v2 06/13] usb: hcd: Add hcd add/remove functions for OTG use
On Fri, 17 Apr 2015, Roger Quadros wrote: On 17/04/15 05:18, Peter Chen wrote: On Tue, Apr 14, 2015 at 01:41:53PM +0300, Roger Quadros wrote: The existing usb_add/remove_hcd() functionality remains unchanged for non-OTG devices. For OTG devices they only register the HCD with the OTG core. Introduce _usb_add/remove_hcd() for use by OTG core. These functions actually add/remove the HCD. Would you please explain why additional _usb_add/remove_hcd are needed? It is to differentiate if the add/remove_hcd was called by the HCD drivers or by the OTG core as we want to behave a bit differently in both cases. When called by HCD drivers, we want to defer the add/remove if it is an OTG device. When called by OTG core, we don't want to defer the add/remove. I don't understand this. Why do you want to defer the add/remove if the device is OTG? Don't host controller drivers expect these things to execute synchronously? For example, what happens if you rmmod the HCD? If the remove call gets deferred, then when it finally runs it will try to call back into HCD code that has already been unloaded from memory! HCD drivers use usb_add/remove_hcd() OTG core uses _usb_add/remove_hcd() How about a more explicit naming scheme? HC drivers use usb_add/remove_hcd() OTG core uses usb_otg_add/remove_hcd() Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
On Thu, 19 Mar 2015, Roger Quadros wrote: It would be a lot simpler to unbind the host controller driver completely when switching to device mode and rebind it when switching back. I guess that is the sort of heavy-duty approach you want to avoid, but it may be the only practical way forward. So you mean directly calling usb_add/remove_hcd() from the OTG core? Well, actually I meant calling device_release_driver() and driver_probe_device(). But usb_add/remove_hcd() might work just about as well, although I think some of the HCDs may still require a little rewriting. Peter Chen says it works for Chipidea. I don't see any issues with that other than it being a heavy-duty operation like you said and hope that it doesn't violate the OTG spec timing. The timing is a tough issue. I'm not sure what we can do about it, though. Looking at Figure 5-3: HNP Sequence of Events (FS) of the OTG 2.0 spec we have about 150ms (X10) to switch from B-Device detected A connect (b_wait_acon state) to driving bus reset (b_host state). I don't think this should be an issue in modern SoCs but I'm not very sure. In any case I can migrate to the add/remove hcd approach to simplify things. Good luck. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
On Wed, 18 Mar 2015, Tony Lindgren wrote: If the host controller is started more than once, you will end up unregistering and re-registering the root hub. The device core does not allow this. Once a device has been unregistered, you must not try to register it again -- you have to allocate a new device and register it instead. Also, although you call the driver's -start method multiple times, the -reset method is called only once, when the controller is first probed. It's not clear that this will work in a situation where the HC and the UDC share hardware state; after the UDC is stopped it may be necessary to reset the HC before it can run again. It might be possible to make this work, but I suspect quite a few drivers would need rewriting first. As another example of the problems you face, consider how stopping a host controller will interact with the driver's PM support (both system suspend and runtime suspend). It would be a lot simpler to unbind the host controller driver completely when switching to device mode and rebind it when switching back. I guess that is the sort of heavy-duty approach you want to avoid, but it may be the only practical way forward. Hmm from memory I think the OTG spec assumes the USB devices are suspended when attempting the role change? I could be totally wrong, it's been a really long time since I've looked at the OTG spec, but maybe that would make it easier to deal with thing? This patch deals with the host side, not the device side. The fact that the device is suspended is not relevant to the issues above. Besides, the problems I outlined are more connected with the way Linux's host-side USB stack is organized, and not so much with the details of the OTG spec. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH 1/9] usb: hcd: Introduce usb_start/stop_hcd()
On Wed, 18 Mar 2015, Roger Quadros wrote:
To support OTG we want a mechanism to start and stop
the HCD from the OTG state machine. Add usb_start_hcd()
and usb_stop_hcd().
Signed-off-by: Roger Quadros rog...@ti.com
There are a few problems in this proposed patch.
+int usb_start_hcd(struct usb_hcd *hcd)
+{
+ int retval;
+ struct usb_device *rhdev = hcd-self.root_hub;
+
+ if (hcd-state != HC_STATE_HALT) {
+ dev_err(hcd-self.controller, not starting a running HCD\n);
+ return -EINVAL;
+ }
+
+ hcd-state = HC_STATE_RUNNING;
+ retval = hcd-driver-start(hcd);
+ if (retval 0) {
+ dev_err(hcd-self.controller, startup error %d\n, retval);
+ hcd-state = HC_STATE_HALT;
+ return retval;
+ }
+
+ /* starting here, usbcore will pay attention to this root hub */
+ if ((retval = register_root_hub(hcd)) != 0)
+ goto err_register_root_hub;
If the host controller is started more than once, you will end up
unregistering and re-registering the root hub. The device core does
not allow this. Once a device has been unregistered, you must not try
to register it again -- you have to allocate a new device and register
it instead.
Also, although you call the driver's -start method multiple times, the
-reset method is called only once, when the controller is first
probed. It's not clear that this will work in a situation where the HC
and the UDC share hardware state; after the UDC is stopped it may be
necessary to reset the HC before it can run again.
It might be possible to make this work, but I suspect quite a few
drivers would need rewriting first. As another example of the problems
you face, consider how stopping a host controller will interact with
the driver's PM support (both system suspend and runtime suspend).
It would be a lot simpler to unbind the host controller driver
completely when switching to device mode and rebind it when switching
back. I guess that is the sort of heavy-duty approach you want to
avoid, but it may be the only practical way forward.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap 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/4] PM / Wakeirq: Add minimal device wakeirq helper functions
On Mon, 9 Mar 2015, Tony Lindgren wrote: Considering the above, should we add a new function something like pm_resume_complete() that does not need irq_safe set but does not return until the device has completed resume? That doesn't make sense. You're asking for a routine that is allowed to sleep but can safely be called in interrupt context. Oh it naturally would not work in irq context, it's for the bottom half again. In other words, you're suggesting we add a function that runs in process context and doesn't return until the device is fully resumed? That's exactly what pm_runtime_resume does right now. But doesn't it only wait for completion if the driver is marked with pm_runtime_irq_safe()? Put it this way: pm_runtime_resume invokes a -runtime_resume callback routine (the subsystem's or the driver's or whichever), and it assumes that if the routine returns 0 then the device has been resumed. It doesn't really _wait_ for anything; it just calls the callback routine. It behaves this way whether or not the irq_safe flag is set. The only difference is that if irq_safe is set then the callback routine is invoked with interrupts disabled (and in this case pm_runtime_resume may be called in interrupt context -- normally it can be called only in process context). Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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/4] PM / Wakeirq: Add minimal device wakeirq helper functions
On Sat, 7 Mar 2015, Rafael J. Wysocki wrote: Well right now it's using threaded irq, and I'd like to get rid of I'll verify again, but I believe the issue was that without doing mark_last_busy here the device falls back asleep right away. That probably should be fixed in pm_runtime in general if that's the case. It's up to the subsystem to handle this. For example, the USB subsystem's runtime-resume routine calls pm_runtime_mark_last_busy. I'm wondering, though, if there's any reason for us to avoid updating power.last_busy in rpm_resume(). If I was a driver writer, I'd expect the core to do that for me quite frankly. In theory, you might want to wake up a device to perform some very limited operation (like reading an internal register) and then put it back into suspend very quickly, without waiting for the autosuspend delay to elapse. Apart from that, I can't think of any reason not to update last_busy in rpm_resume. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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/4] PM / Wakeirq: Add minimal device wakeirq helper functions
On Fri, 6 Mar 2015, Tony Lindgren wrote: I'll verify again, but I believe the issue was that without doing mark_last_busy here the device falls back asleep right away. As it should. If you don't increment the usage counter (for example, by calling pm_runtime_get instead of pm_request_resume) and you don't update last_busy then you are telling the PM core that the device currently isn't busy and it hasn't been in use since the last time it was suspended. Under those circumstances, the PM core is _supposed_ to suspend the device right away. That probably should be fixed in pm_runtime in general if that's the case. It's up to the subsystem to handle this. For example, the USB subsystem's runtime-resume routine calls pm_runtime_mark_last_busy. Hmm.. OK thanks this probably explains why pm_request_resume() did not work. For omaps, I could call this from the interconnect related code, but then how dow we deal with the subsystems that don't call it? Start by determining _why_ they don't call it. Maybe they have a good reason. If they don't then fix them. Considering the above, should we add a new function something like pm_resume_complete() that does not need irq_safe set but does not return until the device has completed resume? That doesn't make sense. You're asking for a routine that is allowed to sleep but can safely be called in interrupt context. Oh it naturally would not work in irq context, it's for the bottom half again. In other words, you're suggesting we add a function that runs in process context and doesn't return until the device is fully resumed? That's exactly what pm_runtime_resume does right now. But I'll take a look if we can just call pm_request_resume() and disable_irq() on the wakeirq in without waiting for the device driver runtime_suspend to disable the wakeirq. That would minimize the interface to just dev_pm_request_wakeirq() and dev_pm_free_wakeirq(). Will that be acceptable in systems with shared IRQ lines? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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/4] PM / Wakeirq: Add minimal device wakeirq helper functions
On Sat, 7 Mar 2015, Rafael J. Wysocki wrote:
But this is part of a bigger picture. Namely, if a separete wakeup interrupt
is required for a device, the device's power.can_wakeup flag cannot be set
until that interrupt has been successfully requested. Also for devices that
can signal wakeup via their own IO interrupts, it would make sense to allow
those interrupts to be registered somehow as wakeup interrupts.
So I wonder if we can define a new struct along the lines of your
struct wakeirq_source, but call it struct wake_irq and make it look
something like this:
struct wake_irq {
struct device *dev;
int irq;
irq_handler_t handler;
};
Then, add a struct wake_irq pointer to struct dev_pm_info *and* to
struct wakeup_source. Next, make dev_pm_request_wake_irq() allocate the
structure and request the interrupt and only set the pointer to it from
struct dev_pm_info *along* *with* power.can_wakeup if all that was
successful.
For devices that use their own IO IRQ for wakeup, we can add something
like dev_pm_set_wake_irq() that will work analogously, but without requesting
the interrupt. It will just set the dev and irq members of struct wake_irq
and point struct dev_pm_info to it and set its power.can_wakeup flag.
Then, device_wakeup_enable() will be able to see that the device has a
wakeup IRQ and it may then point its own struct wake_irq pointer to that.
The core may then use that pointer to trigger enable_irq_wake() for the
IRQ in question and it will cover the devices that don't need separate
wakeup interrupts too.
Does that make sense to you?
Can we back up a little? What is the basic problem the two of you are
trying to solve?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap 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/4] PM / Wakeirq: Add minimal device wakeirq helper functions
On Fri, 6 Mar 2015, Tony Lindgren wrote:
+ struct wakeirq_source *wirq = _wirq;
+ irqreturn_t ret = IRQ_NONE;
+
+ /* We don't want RPM_ASYNC or RPM_NOWAIT here */
+ if (pm_runtime_suspended(wirq-dev)) {
What if the device is resumed on a different CPU right here?
Good point, sounds like we need to do this in some pm_runtime
function directly for the locking.
+ pm_runtime_mark_last_busy(wirq-dev);
+ pm_runtime_resume(wirq-dev);
Calling this with disabled interrupts is a bad idea in general.
Is the device guaranteed to have power.irq_safe set?
Well right now it's using threaded irq, and I'd like to get rid of
the pm_runtime calls in the regular driver interrupts completely.
We need to ensure the device runtime_resume is completed before
returning IRQ_HANDLED here.
In general, runtime_resume methods are allowed to sleep. They can't be
used in an interrupt handler top half unless the driver has
specifically promised they are IRQ-safe. That's what Rafael was
getting at.
Of course, if this routine is a threaded-irq bottom half then there's
no problem.
I guess what you want to call here is pm_request_resume() and
I wouldn't say that calling pm_runtime_mark_last_busy() on a
suspended device was valid.
I'll verify again, but I believe the issue was that without doing
mark_last_busy here the device falls back asleep right away.
That probably should be fixed in pm_runtime in general if that's
the case.
It's up to the subsystem to handle this. For example, the USB
subsystem's runtime-resume routine calls pm_runtime_mark_last_busy.
Considering the above, should we add a new function something like
pm_resume_complete() that does not need irq_safe set but does
not return until the device has completed resume?
That doesn't make sense. You're asking for a routine that is allowed
to sleep but can safely be called in interrupt context.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/5] xhci: add a quirk for device disconnection errata for Synopsis Designware USB3 core
On Thu, 12 Feb 2015, Mathias Nyman wrote: On 25.01.2015 10:13, Sneeker Yeh wrote: This issue is defined by a three-way race at disconnect, between 1) Class driver interrupt endpoint resheduling attempts if the ISR gave an ep error event due to device detach (it would try 3 times) 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread asynchronously 3) The hardware IP was configured in silicon with - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 - Synopsys IP version is 3.00a The IP will auto-suspend itself on device detach with some phy-specific interval after CSC is cleared by 2) If 2) and 3) complete before 1), the interrupts it expects will not be generated by the autosuspended IP, leading to a deadlock. Even later disconnection procedure would detect that corresponding urb is still in-progress and issue a ep stop command, auto-suspended IP still won't respond to that command. If the Synopsys IP provides a way to do it, it would be better to turn off the autosuspend feature entirely. Doesn't autosuspend violate the xHCI specification? So did I understand correctly that the class driver submits a new urb which is enqueued by xhci_urb_enqueue() before the hub thread notices the device is disconnected. Then hub thread clears CSC bit, controller suspends and the new urb is never given back? Doesn't the CSC bit and PORT_CONNECT bit show the device is disconnected when we enter xhci_enqueue_urb(), even it the hub thread doesn't know this yet? What if the device disconnects _after_ the new URB is enqueued? Would it make sense to check those bits in xhci_enqueue_urb, and just return error in the xhci_urb_enqueue() if device is not connected? Then there wouldn't be a need for any quirk at all. That wouldn't help URBs that were already enqueued when the disconnect occurred. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: OMAP3/AM3517 EHCI USB Issue
On Tue, 29 Jul 2014, Michael Welling wrote: Today I enabled debugging in the core hub driver and found that once the external HUB suspends, the root HUB suspends. Naturally. You can prevent that by forcing the root hub to remain active. Once the root HUB suspends, it seems there is no way for the external HUB to wake the root HUB with the hardware setup that I have. Here is what happens when I echo on into the power control for the HUB afterwards: root@som3517:/# echo on /sys/bus/usb/devices/1-1/power/control [ 1050.003689] hub 1-0:1.0: hub_resume [ 1050.011306] usb usb1-port1: status 0507 change [ 1050.019975] hub 1-0:1.0: state 7 ports 3 chg evt [ 1050.028076] usb 1-1: usb auto-resume [ 1050.065904] hub 1-0:1.0: state 7 ports 3 chg evt 0002 [ 1050.084623] usb 1-1: finish resume [ 1050.092469] usb 1-1: retry with reset-resume [ 1050.156167] hub 1-0:1.0: port_wait_reset: err = -16 [ 1050.161331] usb usb1-port1: not enabled, trying reset again... [ 1050.376402] usb usb1-port1: logical disconnect [ 1050.381354] usb 1-1: gone after usb resume? status -19 [ 1050.386941] usb 1-1: can't resume, status -19 [ 1050.391537] usb usb1-port1: logical disconnect [ 1050.400203] usb usb1-port1: status 0100, change 0003, 12 Mb/s [ 1050.406410] usb 1-1: USB disconnect, device number 2 [ 1050.411650] usb 1-1: unregistering device [ 1050.604563] usb usb1-port1: debounce total 100ms stable 100ms status 0x100 [ 1050.611901] hub 1-0:1.0: state 7 ports 3 chg evt [ 1050.618428] hub 1-0:1.0: hub_suspend The hub disconnects. Further research led me to find multiple errata that may be causing this issue. www.ti.com/lit/pdf/sprz306 That 1.1.18 advisory looks pretty bad. Still does not explain why the same hardware works with the 2.6.37 and 3.2 kernels. Maybe you should try bisection. It's slow but it gives you an answer. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Thu, 8 May 2014, Paul Zimmerman wrote: That doesn't handle the problem I described above. When the dwc3 driver gets the late delayed status response, it will think it is a response to the new SETUP packet, and so it will carry out a bogus transfer. It won't know that the status request was meant to be a response to a defunct control transfer. I think you misunderstood me. What I meant was, once the DWC3 controller sees the next SETUP packet, it will still accept the commands from the dwc3 driver to start the DATA and STATUS phases for the previous Control transfer, and will send back (fake) completion events for those commands to the driver. But it won't actually send anything on the wire. I see. The hardware keeps track of which transfer is being acted on. So it should be impossible for the dwc3 driver to carry out a bogus transfer. This is a feature of the DWC3 controller intended to simplify what the software needs to handle, and to automatically take care of the corner cases involved here. For other controllers that can't do this, maybe it should be handled in the UDC driver rather than in the composite gadget? The only place this can be handled properly is in the gadget driver: composite.c for those gadgets using it, otherwise in the higher level driver (if there are any remaining gadgets that don't use the composite framework). Why can't the UDC drivers handle this? AFAIK they all keep track of which phase of a Control transfer they are in. If they see another SETUP packet arrive, they could fake the DATA/STATUS stages of the previous transfer, before passing on the next SETUP packet to the gadget driver. Similar to what the DWC3 controller does in hardware. That would be possible, yes. Although, I guess it would be simpler to do it once in composite.c, instead of in each individual UDC driver. But there would have to be a quirk or something, to disable the code if the dwc3 driver is in use. And that wouldn't fix the problem for gadgets that don't use composite.c. Would the dwc3 driver really need such a quirk? From what you said before, I got the impression that if a new SETUP packet arrived before the old transfer was complete, dwc3 wouldn't inform the gadget driver about it. It would wait until the gadget driver submitted its status response for the old transfer and _then_ tell it about the new SETUP. As for gadgets not using the composite framework, I don't think there are very many of them left. gadgetfs certainly, but hardly any others. Felipe should know for sure. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Thu, 8 May 2014, Zhuang Jin Can wrote: When the host already timed out the control transfer and started a new one. Here's what I'm talking about: Host sends a Set-Configuration request. The UDC driver calls the gadget driver's setup function. The setup function returns DELAYED_STATUS. After a few seconds, the host gets tired of waiting and sends a Get-Descriptor request My understanding is dwc3 will return NYET to host for this Get-Descriptor request transaction, as dwc3 is still in STATUS phase, there's no buffer to receive anything in ep0-out. dwc3 _cannot_ return NYET to a SETUP packet. The USB protocol does not allow it. A device must always respond to SETUP with ACK. On the other hand, dwc3 _can_ return NYET to the token packet that follows the SETUP transaction. That's what it should do. But at this point it should be in the DATA stage, not the STATUS stage. Receiving a SETUP packet should abort a STATUS stage. And your below comments is not applicapable to dwc3. True, they apply to composite.c rather than dwc3. However, they address an issue very similar to your patch, so I raised this topic in your email thread. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Thu, 8 May 2014, Zhuang Jin Can wrote: dwc3 _cannot_ return NYET to a SETUP packet. The USB protocol does not allow it. A device must always respond to SETUP with ACK. It true that device can not return NYET to a SETUP packet. A device must always respond to SETUP with ACK _if_ the SETUP packet is correctly received. Because there's no buffer prepared in ep0 for dwc3 to receive the SETUP packet, I guess there will be no handshake returned to host. I can confirm this by doing an experiment tomorrow:) The dwc3 driver should always prepare a buffer for the next ep0 SETUP packet as soon as it retrieves the information for the current SETUP packet from the buffer. Otherwise, as you described it, if the gadget driver never sends its delayed status response then the UDC will never respond to any more control transfers. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] usb: ohci: sort out dependencies for lpc32xx and omap
On Thu, 8 May 2014, Arnd Bergmann wrote: The dependency on the isp1301 driver is not something that should be in the main OHCI driver but rather the SoC specific part of it. This moves the dependency for LPC32xx into USB_OHCI_HCD_LPC32XX, and changes the 'select ISP1301_OMAP' to a similar 'depends on'. Since the same dependency exists for the client driver, do the same change there. Signed-off-by: Arnd Bergmann a...@arndb.de Cc: linux-omap@vger.kernel.org Cc: Alan Stern st...@rowland.harvard.edu For the host side changes: Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Thu, 8 May 2014, Paul Zimmerman wrote: When the host already timed out the control transfer and started a new one. Here's what I'm talking about: Host sends a Set-Configuration request. The UDC driver calls the gadget driver's setup function. The setup function returns DELAYED_STATUS. After a few seconds, the host gets tired of waiting and sends a Get-Descriptor request The gadget driver finally submits the delayed request response to the Set-Configuration request. But it is now too late, because the host expects a response to the Get-Descriptor request. dwc3 can't move to SETUP phase until the status request arrives, so any SETUP transaction from host will fail. If status request eventually arrives, it already missed the first control transfer, and I don't know how the controller will behave. If we still can get a STATUS XferComplete event without actually transfer anything on the bus, then we can move back to SETUP PHASE which will remove the stale delayed status request and start the new SETUP transaction. But I think in this situation, the host should already lose it patience and start to reset the bus. My point is that the UDC driver can't handle this. Therefore the gadget driver has to prevent this from happening. That means composite.c has to avoid sending delayed status responses if a new SETUP packet has been received already. Per my understanding, it's impossible for dwc3 to send a stale STATUS request for a new SETUP transaction. dwc3 won't know that the status response is stale. It will think the response was meant for the new transfer, not the old one. The DWC3 controller will actually handle this case on its own. If it sees another SETUP packet come in before the previous Control transfer has completed, it will not send any DATA or STATUS phase packets for the previous Control transfer to the host. But it will fake the correct responses to the software, so the dwc3 driver will think that the DATA/STATUS stages completed successfully, even though nothing actually went out on the bus. That doesn't handle the problem I described above. When the dwc3 driver gets the late delayed status response, it will think it is a response to the new SETUP packet, and so it will carry out a bogus transfer. It won't know that the status request was meant to be a response to a defunct control transfer. For other controllers that can't do this, maybe it should be handled in the UDC driver rather than in the composite gadget? The only place this can be handled properly is in the gadget driver: composite.c for those gadgets using it, otherwise in the higher level driver (if there are any remaining gadgets that don't use the composite framework). Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Wed, 7 May 2014, Zhuang Jin Can wrote: A delayed status request may be queued before composite framework returns USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run on a different core in parallel with the control request irq. SETUP XferComplete IRQfsg_main_thread ----- | | spin_lock_irqsave(dwc-lock) sleeping | | ... ... dwc3_ep0_inspect_setup() | | | dwc3_ep0_delegate_req() | | | ... | spin_unlock(dwc-lock); | | | fsg_set_alt() == Signal Wakeup | | | other gadgets-set_alt() handle exception | | | usb_composite_setup_continue() | | | spin_lock_irqsave(dwc-lock) |__dwc3_gadget_ep0_queue() |delay_status is false | spin_unlock_irqrestore(dwc-lock) | | |sleeping spin_lock(dwc-lock);| | | delayed_status=true | | | STATUS XferNotReady IRQ | dwc3_ep0_xfernotready() | delayed_status is true, return; The result is the status packet will never be transferred, and delayed_status is not cleared. Signed-off-by: Zhuang Jin Can jin.can.zhu...@intel.com Reported-by: Zhou Liping liping.z...@intel.com A similar problem can occur in the opposite sense: The thread queuing the delayed status request might be delayed for so long that another SETUP packet arrives from the host first. In that case, the delayed status request is a response for a stale transfer, so it must not be sent to the host. Do dwc3 and composite.c handle this case correctly? Back in the old g_file_storage driver, I addressed this issue by keeping a counter of all the setup requests. When it came time to send a delayed status response, the response would be sent only if the counter had not changed from when the original setup request was received. As far as I can see, composite.c doesn't do anything like that. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: ep0: fix delayed status is queued too early
On Thu, 8 May 2014, Zhuang Jin Can wrote: A similar problem can occur in the opposite sense: The thread queuing the delayed status request might be delayed for so long that another SETUP packet arrives from the host first. In that case, the delayed status request is a response for a stale transfer, so it must not be sent to the host. Do dwc3 and composite.c handle this case correctly? So the situation you describe is that we get the STATUS XferNotReady event, but gadget queues a status request when control transfer already failed. When the host already timed out the control transfer and started a new one. Here's what I'm talking about: Host sends a Set-Configuration request. The UDC driver calls the gadget driver's setup function. The setup function returns DELAYED_STATUS. After a few seconds, the host gets tired of waiting and sends a Get-Descriptor request The gadget driver finally submits the delayed request response to the Set-Configuration request. But it is now too late, because the host expects a response to the Get-Descriptor request. dwc3 can't move to SETUP phase until the status request arrives, so any SETUP transaction from host will fail. If status request eventually arrives, it already missed the first control transfer, and I don't know how the controller will behave. If we still can get a STATUS XferComplete event without actually transfer anything on the bus, then we can move back to SETUP PHASE which will remove the stale delayed status request and start the new SETUP transaction. But I think in this situation, the host should already lose it patience and start to reset the bus. My point is that the UDC driver can't handle this. Therefore the gadget driver has to prevent this from happening. That means composite.c has to avoid sending delayed status responses if a new SETUP packet has been received already. Per my understanding, it's impossible for dwc3 to send a stale STATUS request for a new SETUP transaction. dwc3 won't know that the status response is stale. It will think the response was meant for the new transfer, not the old one. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: gadget: fix burst size corruption
On Thu, 1 May 2014, Zhuang Jin Can wrote: again, you found a bug on the gadget driver. Fix that. composite.c guarantees that for those functions which don't pass bMaxBurst, gadget-maxburst will be set to *at least* 1. I agree the real fix should be in the gadget driver. The patch intents to prevent hibernatition from being corrupted by a bad gadget driver. If OEMs develop their own gadget driver forgetting to call config_ep_by_speed(), it'll turn out to be everything works except dwc3 hibernation, and they'll complain to dwc3. f_ffs is an example has SuperSpeed support but doesn't call config_ep_by_speed(). It's just for robustness, and dwc3 is not doing anything wrong. It did cause me a long time to figure out why the hibernation was broken. You could include the check, for the sake of robustness, in dwc3 -- but if it fails, you should write a message to the kernel log saying that the gadget driver needs to be fixed. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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/2] usb: rename 'phy' field of 'struct usb_hcd' to 'transceiver'
On Wed, 9 Apr 2014, Sergei Shtylyov wrote: Ok, the existing field is being replaced by something? I didn't get that No, not replaced. I'm adding the support for generic PHY to the existing USB PHY support. I thought that was clear from the changelog. from the patch description; I thought the new name in this patch was going to be it. In that case, a temporary name of usb_phy for the existing field, or adding the new field as gen_phy sound reasonable. OK, I'll respin the patch #2 with 'gen_phy' and remove the patch #1. What is the reason for all of this? That is, can you explain the difference between USB PHY support and general PHY support, and why we need both? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: regression(ti platforms): next-20140210 (ehci?)
On Mon, 10 Feb 2014, Kevin Hilman wrote:
The issue started I think with the following patch getting merged:
ehci-platform: Add support for clks and phy passed through devicetree
some version of http://www.spinics.net/lists/linux-usb/msg101061.html
introduced { .compatible = usb-ehci, },
This is what I was getting at: an understanding of what caused the
failue in the first place.
Now, in the build we have two drivers which dts claims compatibility
with, but only 1 driver actually works (drivers/usb/host/ehci-omap.c)
for the platform. Thinking that way, in fact, the current
compatibility even matches drivers/usb/host/ehci-ppc-of.c which
obviously wont work either.
Right, so I agree that it makes sense to remove a compatible string
where there is no compatability, but a couple other things should happen
here.
1) changelog should describe why this compatible string is in the omap
dtsi files in first place.
2) investigation into the patch that introduced this change to double
check it's not introducing other breakage as well.
Oddly enough, it was Roger who introduced this incorrect compatibility
string in commit f17c89948dcd6 (ARM: dts: OMAP4: Add HS USB Host IP
nodes). There is no explanation in the commit log of why the string is
there.
We better remove it from all the files where it doesn't belong. Of
course, this won't help existing hardware. It will still be necessary
to change the compatibility string used for the generic platform
drivers.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] base: platform: add generic clock handling for platform-bus
On Fri, 31 Jan 2014, Felipe Balbi wrote: Still TODO a commit log. Not for merging! NYET-Signed-off-by: Felipe Balbi ba...@ti.com --- This patch is an idea I've had recently in order to combine several different PM implementations into the platform-bus. This patch is bare minimum for platforms which need to handle functional and interface clocks but the whole thing is made optional. Note that this patch makes sure that by the time a platform_driver's probe is called, we already have clocks enabled and pm_runtime_set_active() has been called, thus making sure that a device driver's pm_runtime_get_sync() will solely increase the pm usage counter. I have *NOT* tested this anywhere *YET*, but I suppose it shouldn't cause any issues since the clock API has ref counting too. Would really like to get some review from several folks involved with ARM SoC PM so that's the reason for the wide audience. If I have missed anybody, please add them to Cc. As mentioned above, this is *NOT* meant for merging, but serves as a starting point for discussing some convergence of several PM domain implementations on different arch/arm/mach-* directories. You might want to copy the runtime-PM approach used by the PCI subsystem. It works like this: The core invokes a driver's probe routine with runtime PM enabled, the device in the ACTIVE state, and the usage counter incremented by 1. If the driver wants to support runtime PM, the probe routine can call pm_runtime_put_noidle. The core does pm_runtime_get_sync before invoking the driver's remove routine. At this point a runtime-PM-aware driver whould call pm_runtime_get_noresume, to balance the _put during probe. After invoking the remove routine, the core does a put_noidle (to balance the get_sync) and a final put_sync (to balance the increment before probe and to leave the device at low power.) A nice consequence is that everything is transparent for drivers that don't support runtime PM. The usage counter remains 0 the entire time the driver is bound. Conversely, drivers that do support runtime PM merely have to add one call during probe and one during remove. There is one tricky aspect to all this. The driver core sets the dev-driver field before calling the subsystem core's probe routine. As a result, the subsystem has to be very careful about performing runtime PM before invoking the driver's probe routine. Otherwise you will end up calling the driver's runtime_resume callback before the driver's probe! (And of course, the same problem exists in reverse during remove.) Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend
On Mon, 25 Nov 2013, Felipe Balbi wrote: On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: Make musb_port_suspend() externally available, and call it when to host goes into suspend. This allows the core to go into suspend while a device is connected. have you considered the fact that when musb looses context it'll cause a disconnect on the bus because soft_connect bit is lost ? What if you have a mounted file system on a pendrive ? Should we allow suspend in that case ? Alan, Greg, any hints ? An HCD should strive to avoid losing power sessions over a suspend/resume. But if this is unavoidable, the USB Persist mechanism should be able to cope by issuing a reset-resume. (Unless userspace has turned Persist off, of course -- I believe some distributions do this. Chrome?) Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend
On Mon, 25 Nov 2013, Daniel Mack wrote: What if you have a mounted file system on a pendrive ? Should we allow suspend in that case ? Well, I would have expected that, but in fact, the opposite is true. With 3.12, mounting a filesystem on a USB media and accessing it after resume was exactly my test case, and it worked just fine with that patch. so you had a mounted file system, suspend, resume, it was still mounted? Yes, exactly. or did it reenumerate and you remounted it ? No, it did not reenumerate, but the core did a reset on it. See the following log. ... [ 32.030792] usb 1-1: reset high-speed USB device number 2 using musb-hdrc ! Like I said, you get a reset-resume. Try to do the same with transfers in flight, it's likely to corrupt your file system. It's not so easy to suspend a system with USB mass-storage transfers in flight. For one thing, userspace gets frozen before the suspend starts, so there's nothing to initiate new I/O requests (except perhaps for dirty-block writebacks). Also, the child SCSI device gets suspended before the USB device, so its I/O queue stops running. In addition, the usb-storage suspend routine and the main I/O thread are mutually exclusive (they both grab the private mutex), so a suspend can't occur while a SCSI command is underway. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] usb: musb: call musb_port_suspend from musb_bus_suspend
On Wed, 2 Oct 2013, Daniel Mack wrote: On 02.10.2013 14:01, Sebastian Andrzej Siewior wrote: On 10/02/2013 01:14 PM, Daniel Mack wrote: On 02.10.2013 12:49, Sebastian Andrzej Siewior wrote: What happens if the device is unplugged while the host is suspended and not there on resume? That condition is detected and a full teardown of the connected drivers is conducted. Try what happens on your notebook when you do that. Embedded systems should behave just the same. I had the feeling that the USB device gets disconnected and re-enumerated on resume. Yes. The device looses its +5V power supply, so it has to be reset and re-enumerated on resume of course. It's just that USB drivers know about the state the device has to be put back into after resume, and don't assume the device was disconnected and reconnected. The latter would (for usb-storage) also cause paritions to be removed and added. This description may be slightly misleading. According to the USB spec, a device is not supposed to lose its +5V power supply during suspend. Therefore it does not need to be reset and re-enumerated upon resume. (Except that some devices don't handle suspend properly because of buggy firmware; they _do_ need to be reset.) Of course, some systems don't follow the spec. If they can't supply suspend power to the host controller and the USB bus, then the device _will_ need to be reset and re-enumerated. Some USB drivers (those which define a reset_resume method) are able to handle this -- they put the device back into the right state and then act as if nothing had happened. Others aren't; they get unbound and rebound just as though the device really had been disconnected and reconnected. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
On Tue, 23 Jul 2013, Roger Quadros wrote:
+ pm_runtime_get_sync(dev);
+ ehci_resume(hcd, false);
+ ret = ehci_suspend(hcd, do_wakeup);
+ pm_runtime_put_sync(dev);
It would be better to call pm_runtime_resume(dev) at the start instead
of pm_runtime_get_sync(), and eliminate the last two lines. Then the
ehci_suspend() below could be moved outside the if statement.
Why do I find pm_runtime_* so confusing ;)
It tries to provide every service anyone might want, and ends up being
confusingly large.
In this case, though, the reasoning is simple. We know that after the
system resumes, the device will be back in the active state. Hence
there's no point in calling pm_runtime_put_sync here, other than to
balance the _get_sync above. Since there's no danger of another thread
trying to do a runtime suspend at this point, you don't need to
increment the usage counter. Therefore pm_runtime_resume is good
enough.
Alternatively, if you are able to turn off the wakeup setting without
going through an entire resume/suspend cycle, that would be preferable.
As we don't rely on the EHCI controller's interrupt any more after we
power it down, maybe ehci_resume/suspend cycle is not required at all on OMAP,
even if the wakeup setting is disabled during system suspend.
As the wakeup is controlled entirely by pad wakeup, all we need to do is make
sure
the IO pad wakeup is configured correctly based on do_wakeup. How this is done
is still in transition but it might be turn out to be as simple as
irq_set_irq_wake()
So IMHO, for ehci-omap this should suffice
static int omap_ehci_suspend(struct device *dev)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
bool do_wakeup = device_may_wakeup(dev);
int ret = 0;
if (!pm_runtime_status_suspended(dev))
ret = ehci_suspend(hcd, do_wakeup);
/* Not tested yet */
irq_set_irq_wake(hcd-irq, do_wakeup);
return ret;
}
What do you think?
Nice and simple. It looks good.
As the OMAP IO pad wakeup management code [1] is still in transition, I'll
wait till
that gets settled and then resend this series.
Okay.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote:
On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
Hi Alan,
Thanks for helping to clarify the issues here.
Okay. Are PHYs _always_ platform devices?
They can be i2c, spi or any other device types as well.
In those other cases, presumably there is no platform data associated
with the PHY since it isn't a platform device. Then how does the
kernel know which controller is attached to the PHY? Is this spelled
out in platform data associated with the PHY's i2c/spi/whatever parent?
PHY. Currently this information is represented by name or
ID
strings embedded in platform data.
right. It's embedded in the platform data of the controller.
It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?
By using a PHY lookup as Stephen and I suggested in our previous
replies. Without any extra data in platform data. (I have even posted a
code example.)
I don't understand, because I don't know what a PHY lookup does.
In this case, it doesn't matter where the platform_device structures
are created or where the driver source code is. Let's take a simple
example. Suppose the system design includes a PHY named foo. Then
the board file could contain:
struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);
and a header file would contain:
extern struct phy_info phy_foo;
The PHY supplier could then call phy_create(phy_foo), and the PHY
client could call phy_find(phy_foo). Or something like that; make up
your own structure tags and function names.
It's still possible to have conflicts, but now two PHYs with the same
name (or a misspelled name somewhere) will cause an error at link
time.
This is incorrect, sorry. First of all it's a layering violation - you
export random driver-specific symbols from one driver to another. Then
No, that's not what I said. Neither the PHY driver nor the controller
driver exports anything to the other. Instead, both drivers use data
exported by the board file.
imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2 and
there are two types of consumer drivers (e.g. USB host controllers). Now
consider following mapping:
SoC PHY consumer
A PHY1HOST1
B PHY1HOST2
C PHY2HOST1
D PHY2HOST2
So we have to be able to use any of the PHYs with any of the host
drivers. This means you would have to export symbol with the same name
from both PHY drivers, which obviously would not work in this case,
because having both drivers enabled (in a multiplatform aware
configuration) would lead to linking conflict.
You're right; the scheme was too simple. Instead, the board file must
export two types of data structures, one for PHYs and one for
controllers. Like this:
struct phy_info {
/* Info for the controller attached to this PHY */
struct controller_info *hinfo;
};
struct controller_info {
/* Info for the PHY which this controller is attached to */
struct phy_info *pinfo;
};
The board file for SoC A would contain:
struct phy_info phy1 = {host1);
EXPORT_SYMBOL(phy1);
struct controller_info host1 = {phy1};
EXPORT_SYMBOL(host1);
The board file for SoC B would contain:
struct phy_info phy1 = {host2);
EXPORT_SYMBOL(phy1);
struct controller_info host2 = {phy1};
EXPORT_SYMBOL(host2);
And so on. This explicitly gives the connection between PHYs and
controllers. The PHY providers would use phy1 or phy2, and the PHY
consumers would use host1 or host2.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote:
IMHO it would be better if you provided some code example, but let's try to
check if I understood you correctly.
8
[Board file]
static struct phy my_phy;
static struct platform_device phy_pdev = {
/* ... */
.platform_data = my_phy;
/* ... */
};
static struct platform_device phy_pdev = {
This should be controller_pdev, not phy_pdev, yes?
/* ... */
.platform_data = my_phy;
/* ... */
};
[Provider driver]
struct phy *phy = pdev-dev.platform_data;
ret = phy_create(phy);
[Consumer driver]
struct phy *phy = pdev-dev.platform_data;
ret = phy_get(pdev-dev, phy);
Or even just phy_get(pdev-dev), because phy_get() could be smart
enough to to set phy = dev-platform_data.
8
Is this what you mean?
That's what I was going to suggest too. The struct phy is defined in
the board file, which already knows about all the PHYs that exist in
the system. (Or perhaps it is allocated dynamically, so that when many
board files are present in the same kernel, only the entries listed in
the board file for the current system get created.) Then the
structure's address is stored in the platform data and made available
to both the provider and the consumer.
Even though the struct phy is defined (or allocated) in the board file,
its contents don't get filled in until the PHY driver provides the
details.
It's technically correct, but quality of this solution isn't really nice,
because it's a layering violation (at least if I understood what you mean).
This is because you need to have full definition of struct phy in board file
and a structure that is used as private data in PHY core comes from
platform code.
You don't have to have a full definition in the board file. Just a
partial definition -- most of the contents can be filled in later, when
the PHY driver is ready to store the private data.
It's not a layering violation for one region of the kernel to store
private data in a structure defined by another part of the kernel.
This happens all the time (e.g., dev_set_drvdata).
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote: That's what I was going to suggest too. The struct phy is defined in the board file, which already knows about all the PHYs that exist in the system. (Or perhaps it is allocated dynamically, so that when many board files are present in the same kernel, only the entries listed in the board file for the current system get created.) Well, such dynamic allocation is a must. We don't accept non-multiplatform aware code anymore, not even saying about multiboard. Then the structure's address is stored in the platform data and made available to both the provider and the consumer. Yes, technically this can work. You would still have to perform some kind of synchronization to make sure that the PHY bound to this structure is actually present. This is again technically doable (e.g. a list of registered struct phys inside PHY core). The synchronization takes place inside phy_get. If phy_create hasn't been called for this structure by the time phy_get runs, phy_get will return an error. Even though the struct phy is defined (or allocated) in the board file, its contents don't get filled in until the PHY driver provides the details. You can't assure this. Board file is free to do whatever it wants with this struct. A clean solution would prevent this. I'm not sure what you mean here. Of course I can't prevent a board file from messing up a data structure. I can't prevent it from causing memory access violations either; in fact, I can't prevent any bugs in other people's code. Besides, why do you say the board file is free to do whatever it wants with the struct phy? Currently the struct phy is created by the PHY provider and the PHY core, right? It's not even mentioned in the board file. It's technically correct, but quality of this solution isn't really nice, because it's a layering violation (at least if I understood what you mean). This is because you need to have full definition of struct phy in board file and a structure that is used as private data in PHY core comes from platform code. You don't have to have a full definition in the board file. Just a partial definition -- most of the contents can be filled in later, when the PHY driver is ready to store the private data. It's not a layering violation for one region of the kernel to store private data in a structure defined by another part of the kernel. This happens all the time (e.g., dev_set_drvdata). Not really. The phy struct is something that _is_ private data of PHY subsystem, not something that can store private data of PHY subsystem (sure it can store private data of particular PHY driver, but that's another story) and only PHY subsystem should have access to its contents. If you want to keep the phy struct completely separate from the board file, there's an easy way to do it. Let's say the board file knows about N different PHYs in the system. Then you define an array of N pointers to phys: struct phy *(phy_address[N]); In the platform data for both PHY j and its controller, store phy_address[j]. The PHY provider passes this cookie to phy_create: cookie = pdev-dev.platform_data; ret = phy_create(phy, cookie); and phy_create simply stores: *cookie = phy. The PHY consumer does much the same the same thing: cookie = pdev-dev.platform_data; phy = phy_get(cookie); phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise. By the way, we need to consider other cases here as well, for example it would be nice to have a single phy_get() function that works for both non- DT and DT cases to make the consumer driver not have to worry whether it's being probed from DT or not. You ought to be able to adapt this scheme to work with DT. Maybe by having multiple phy_address arrays. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Tue, 23 Jul 2013, Tomasz Figa wrote: If you want to keep the phy struct completely separate from the board file, there's an easy way to do it. Let's say the board file knows about N different PHYs in the system. Then you define an array of N pointers to phys: struct phy *(phy_address[N]); In the platform data for both PHY j and its controller, store phy_address[j]. The PHY provider passes this cookie to phy_create: cookie = pdev-dev.platform_data; ret = phy_create(phy, cookie); and phy_create simply stores: *cookie = phy. The PHY consumer does much the same the same thing: cookie = pdev-dev.platform_data; phy = phy_get(cookie); phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise. OK, this can work. Again, just technically, because it's rather ugly. There's no reason the phy_address things have to be arrays. A separate individual pointer for each PHY would work just as well. Where would you want to have those phy_address arrays stored? There are no board files when booting with DT. Not even saying that you don't need to use any hacky schemes like this when you have DT that nicely specifies relations between devices. If everybody agrees DT has a nice scheme for specifying relations between devices, why not use that same scheme in the PHY core? Anyway, board file should not be considered as a method to exchange data between drivers. It should be used only to pass data from it to drivers, not the other way. Ideally all data in a board file should be marked as const and __init and dropped after system initialization. The phy_address things don't have to be defined or allocated in the board file; they could be set up along with the platform data. In any case, this was simply meant to be a suggestion to show that it is relatively easy to do what you need without using name or ID strings. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
The PHY and the controller it is attached to are both physical
devices.
The connection between them is hardwired by the system
manufacturer and cannot be changed by software.
PHYs are generally described by fixed system-specific board
files or by Device Tree information. Are they ever discovered
dynamically?
No. They are created just like any other platform devices are created.
Okay. Are PHYs _always_ platform devices?
Is the same true for the controllers attached to the PHYs?
If not -- if both a PHY and a controller are discovered
dynamically -- how does the kernel know whether they are
connected to each other?
No differences here. Both PHY and controller will have dt information or hwmod
data using which platform devices will be created.
The kernel needs to know which controller is attached to which
PHY. Currently this information is represented by name or ID
strings embedded in platform data.
right. It's embedded in the platform data of the controller.
It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?
The PHY's driver (the supplier) uses the platform data to
construct a platform_device structure that represents the PHY.
Currently the driver assigns static labels (corresponding to the label used in
the platform data of the controller).
Until this is done, the controller's driver (the client) cannot
use the PHY.
right.
Since there is no parent-child relation between the PHY and the
controller, there is no guarantee that the PHY's driver will be
ready when the controller's driver wants to use it. A deferred
probe may be needed.
right.
The issue (or one of the issues) in this discussion is that
Greg does not like the idea of using names or IDs to associate
PHYs with controllers, because they are too prone to
duplications or other errors. Pointers are more reliable.
But pointers to what? Since the only data known to be
available to both the PHY driver and controller driver is the
platform data, the obvious answer is a pointer to platform data
(either for the PHY or for the controller, or maybe both).
hmm.. it's not going to be simple though as the platform device for the PHY
and
controller can be created in entirely different places. e.g., in some cases
the
PHY device is a child of some mfd core device (the device will be created in
drivers/mfd) and the controller driver (usually) is created in board file. I
guess then we have to come up with something to share a pointer in two
different files.
The ability for two different source files to share a pointer to a data
item defined in a third source file has been around since long before
the C language was invented. :-)
In this case, it doesn't matter where the platform_device structures
are created or where the driver source code is. Let's take a simple
example. Suppose the system design includes a PHY named foo. Then
the board file could contain:
struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);
and a header file would contain:
extern struct phy_info phy_foo;
The PHY supplier could then call phy_create(phy_foo), and the PHY
client could call phy_find(phy_foo). Or something like that; make up
your own structure tags and function names.
It's still possible to have conflicts, but now two PHYs with the same
name (or a misspelled name somewhere) will cause an error at link time.
Probably some of the details above are wrong; please indicate where I
have gone astray. Also, I'm not clear about the role played by various
APIs. For example, where does phy_create() fit into this picture?
phy_create is the API by which the PHY's driver (the supplier) hook into the
PHY framework. It's like the controller driver will always interact with the
PHY driver through the PHY framework.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
On Mon, 22 Jul 2013, Roger Quadros wrote:
Right, I understand it now. How does the below code look?
+static int omap_ehci_suspend(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ bool do_wakeup = device_may_wakeup(dev);
+ int ret;
+
+ dev_dbg(dev, %s may_wakeup %d\n, __func__, do_wakeup);
+
+ if (pm_runtime_status_suspended(dev)) {
You need to do this only when do_wakeup is false.
+ pm_runtime_get_sync(dev);
+ ehci_resume(hcd, false);
+ ret = ehci_suspend(hcd, do_wakeup);
+ pm_runtime_put_sync(dev);
It would be better to call pm_runtime_resume(dev) at the start instead
of pm_runtime_get_sync(), and eliminate the last two lines. Then the
ehci_suspend() below could be moved outside the if statement.
Alternatively, if you are able to turn off the wakeup setting without
going through an entire resume/suspend cycle, that would be preferable.
+
+ } else {
+ ret = ehci_suspend(hcd, do_wakeup);
+ }
+
+ return ret;
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ int ret;
+
+ dev_dbg(dev, %s\n, __func__);
+
+ ret = ehci_resume(hcd, false);
+ if (!ret) {
+ /*
+* Controller was powered ON so reflect state
+*/
+ pm_runtime_disable(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ }
+
+ return ret;
+}
All good.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sun, 21 Jul 2013, Sylwester Nawrocki wrote: What's wrong with the platform_data structure, why can't that be used for this? At the point the platform data of some driver is initialized, e.g. in board setup code the PHY pointer is not known, since the PHY supplier driver has not initialized yet. Even though we wanted to pass pointer to a PHY through some notifier call, it would have been not clear which PHY user driver should match on such notifier. A single PHY supplier driver can create M PHY objects and this needs to be mapped to N PHY user drivers. This mapping needs to be defined somewhere by the system integrator. It works well with device tree, but except that there seems to be no other reliable infrastructure in the kernel to define links/dependencies between devices, since device identifiers are not known in advance in all cases. What Tomasz proposed seems currently most reasonable to me for non-dt. Or, if not, we can always add pointers to the platform device structure, or even the main 'struct device' as well, that's what it is there for. Still we would need to solve a problem which platform device structure gets which PHY pointer. Can you explain the issues in more detail? I don't fully understand the situation. Here's what I think I know: The PHY and the controller it is attached to are both physical devices. The connection between them is hardwired by the system manufacturer and cannot be changed by software. PHYs are generally described by fixed system-specific board files or by Device Tree information. Are they ever discovered dynamically? Is the same true for the controllers attached to the PHYs? If not -- if both a PHY and a controller are discovered dynamically -- how does the kernel know whether they are connected to each other? The kernel needs to know which controller is attached to which PHY. Currently this information is represented by name or ID strings embedded in platform data. The PHY's driver (the supplier) uses the platform data to construct a platform_device structure that represents the PHY. Until this is done, the controller's driver (the client) cannot use the PHY. Since there is no parent-child relation between the PHY and the controller, there is no guarantee that the PHY's driver will be ready when the controller's driver wants to use it. A deferred probe may be needed. The issue (or one of the issues) in this discussion is that Greg does not like the idea of using names or IDs to associate PHYs with controllers, because they are too prone to duplications or other errors. Pointers are more reliable. But pointers to what? Since the only data known to be available to both the PHY driver and controller driver is the platform data, the obvious answer is a pointer to platform data (either for the PHY or for the controller, or maybe both). Probably some of the details above are wrong; please indicate where I have gone astray. Also, I'm not clear about the role played by various APIs. For example, where does phy_create() fit into this picture? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] drivers: phy: add generic PHY framework
On Sat, 20 Jul 2013, Greg KH wrote: That should be passed using platform data. Ick, don't pass strings around, pass pointers. If you have platform data you can get to, then put the pointer there, don't use a name. I don't think I understood you here :-s We wont have phy pointer when we create the device for the controller no?(it'll be done in board file). Probably I'm missing something. Why will you not have that pointer? You can't rely on the name as the device id will not match up, so you should be able to rely on the pointer being in the structure that the board sets up, right? Don't use names, especially as ids can, and will, change, that is going to cause big problems. Use pointers, this is C, we are supposed to be doing that :) Kishon, I think what Greg means is this: The name you are using must be stored somewhere in a data structure constructed by the board file, right? Or at least, associated with some data structure somehow. Otherwise the platform code wouldn't know which PHY hardware corresponded to a particular name. Greg's suggestion is that you store the address of that data structure in the platform data instead of storing the name string. Have the consumer pass the data structure's address when it calls phy_create, instead of passing the name. Then you don't have to worry about two PHYs accidentally ending up with the same name or any other similar problems. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
On Thu, 11 Jul 2013, Roger Quadros wrote: The other two problems are both related to the interaction between system PM and runtime PM. Suppose the controller is already runtime suspended when the system goes to sleep. Because it is runtime suspended, it is enabled for wakeup. But device_may_wakeup() could return false; if this happens then you have to do a runtime-resume in omap_ehci_suspend() before calling ehci_suspend(), so that the controller can be suspended again with wakeups disabled. (Or you could choose an alternative method for accomplishing the same result, such as disabling the wakeup signal from the pad without going through a whole EHCI resume/suspend cycle.) Conversely, if device_may_wakeup() returns true then you shouldn't do anything at all, because the controller is already suspended with the correct wakeup setting. I think this case is taken care of by the Runtime PM core at least for the OMAP platform according to the documentation http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/power/runtime_pm.txt#n647 No; that section refers only to races, not to wakeup settings. At the end of this mail is the log during system suspend/resume You can notice the following sequence -ehci runtime suspends -system suspend triggered -ehci runtime resumes -ehci suspends (uses new wakeup settings) -system wakeup triggered -ehci resumes -ehci runtime suspends This is because the root hub was runtime suspended with the wrong wakeup setting. The USB core, which is careful about these things, resumed and re-suspended it with the proper wakeup setting. In the process, the controller had to be runtime resumed as well. Try doing the test over again, but this time with the root hub enabled for wakeup and the controller disabled. (I know this is a bizarre combination, but try it anyway.) Also, after the system wakes up, see whether the root hub and controller get runtime suspended. For the third problem, suppose the controller was runtime suspended when the system went to sleep. When the system wakes up, the controller will become active, so you have to inform the runtime PM core about its change of state. Basically, if omap_ehci_resume() sees that ehci_resume() returned 0 then it must do: pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); All of these issues are discussed (among lots of other material) in Documentation/power/runtime_pm.txt. Is this still applicable? Documentation claims During system resume it calls pm_runtime_enable() and pm_runtime_put_sync() for every device right after executing the subsystem-level .resume_early() callback and right after executing the subsystem-level .resume() callback for it, respectively. Yes, this is applicable, but it is irrelevant to the problem I described. You still have to tell the runtime PM core that the device is now active. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers
On Wed, 10 Jul 2013, Roger Quadros wrote:
Some platforms e.g. ehci-omap can generate an interrupt
(i.e. remote wakeup) even when the controller is suspended i.e.
HW_ACCESSIBLE is cleared.
Introduce a flag has_wakeup_irq in struct usb_hcd to indicate
such cases.
We tackle this case by disabling the IRQ, scheduling a
hub resume and enabling back the IRQ after the controller has
resumed. This ensures that the IRQ handler runs only after the
controller is accessible.
@@ -2248,7 +2253,21 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
*/
local_irq_save(flags);
- if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
+ if (unlikely(HCD_DEAD(hcd)))
+ rc = IRQ_NONE;
+ else if (unlikely(!HCD_HW_ACCESSIBLE(hcd) hcd-has_wakeup_irq)) {
+ /*
+ * We got a wakeup interrupt while the controller was
+ * suspending or suspended. We can't handle it now, so
+ * disable the IRQ and resume the root hub (and hence
+ * the controller too).
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+
+ rc = IRQ_HANDLED;
+ } else if (unlikely(!HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
In the interest of minimizing the amount of work needed in the most
common case, this should be written as:
else if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
if (hcd-has_wakeup_irq) {
...
} else
rc = IRQ_NONE;
else if (hcd-driver-irq(hcd) == IRQ_NONE)
rc = IRQ_NONE;
Apart from that, the patch is okay. When you rearrange the logic, you
can add my Acked-by.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] USB: ehci-omap: Implement suspend/resume
On Wed, 10 Jul 2013, Roger Quadros wrote:
Call ehci_suspend/resume() during runtime suspend/resume
as well as system suspend/resume.
Use a flag bound to indicate that the HCD structures are valid.
This is only true between usb_add_hcd() and usb_remove_hcd() calls.
The flag can be used by omap_ehci_runtime_suspend/resume() handlers
to avoid calling ehci_suspend/resume() when we are no longer bound
to the HCD e.g. during probe() and remove();
@@ -293,15 +301,67 @@ static const struct of_device_id omap_ehci_dt_ids[] = {
MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids);
+static int omap_ehci_suspend(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+ dev_dbg(dev, %s\n, __func__);
+
+ return ehci_suspend(hcd, true);
+}
+
+static int omap_ehci_resume(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+
+ dev_dbg(dev, %s\n, __func__);
+
+ return ehci_resume(hcd, false);
+}
There are three problems here. The first is very simple: the wakeup
settings are messed up. Wakeup is supposed to be enabled always for
runtime suspend, whereas it is controlled by device_may_wakeup() for
system suspend. You reversed them.
The other two problems are both related to the interaction between
system PM and runtime PM. Suppose the controller is already runtime
suspended when the system goes to sleep. Because it is runtime
suspended, it is enabled for wakeup. But device_may_wakeup() could
return false; if this happens then you have to do a runtime-resume in
omap_ehci_suspend() before calling ehci_suspend(), so that the
controller can be suspended again with wakeups disabled. (Or you could
choose an alternative method for accomplishing the same result, such as
disabling the wakeup signal from the pad without going through a whole
EHCI resume/suspend cycle.) Conversely, if device_may_wakeup() returns
true then you shouldn't do anything at all, because the controller is
already suspended with the correct wakeup setting.
For the third problem, suppose the controller was runtime suspended
when the system went to sleep. When the system wakes up, the
controller will become active, so you have to inform the runtime PM
core about its change of state. Basically, if omap_ehci_resume() sees
that ehci_resume() returned 0 then it must do:
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
All of these issues are discussed (among lots of other material) in
Documentation/power/runtime_pm.txt.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] USB: Support wakeup IRQ for suspended controllers
On Wed, 10 Jul 2013, Roger Quadros wrote:
Some platforms e.g. ehci-omap can generate an interrupt
(i.e. remote wakeup) even when the controller is suspended i.e.
HW_ACCESSIBLE is cleared.
Introduce a flag has_wakeup_irq in struct usb_hcd to indicate
such cases.
We tackle this case by disabling the IRQ, scheduling a
hub resume and enabling back the IRQ after the controller has
resumed. This ensures that the IRQ handler runs only after the
controller is accessible.
Oh yes, one more thing...
@@ -132,6 +134,7 @@ struct usb_hcd {
unsignedwireless:1; /* Wireless USB HCD */
unsignedauthorized_default:1;
unsignedhas_tt:1; /* Integrated TT in root hub */
+ unsignedhas_wakeup_irq:1; /* Can IRQ when suspended */
Please add a highly visible comment here, warning that has_wakeup_irq
should never be set on systems with shared IRQs. Having both would ...
well, it would indicate a really bad system design.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Wed, 3 Jul 2013, Felipe Balbi wrote: On Wed, Jul 03, 2013 at 04:06:04PM +0300, Roger Quadros wrote: On 07/03/2013 03:57 PM, Felipe Balbi wrote: Hi, On Tue, Jul 02, 2013 at 01:17:58PM -0400, Alan Stern wrote: A PCI-based EHCI controller has two power sources: the core well (which is turned off during suspend) and the auxiliary well (which remains powered). That's how remote wakeup works; it uses the auxiliary well. This, kinda, matches what OMAP tries to do with pad wakeup. Just that pad wakeup sits outside of the device itself. Perhaps we could look into how PCI handles the aux well and take some inspiration from there. Any pointers under drivers/pci/ would be great :-) From what I understood, auxiliary well is just a power source, and it keeps the EHCI controller powered even during suspend. If that is true then it is different from our situation as we power down the EHCI controller completely. right but our auxiliary well keeps PRCM powered which can wake EHCI up ;-) What I'm saying is that from ehci-omap's perspective, there's very little difference, specially since we route the wakeup through the same IRQ line anyway. Perhaps we could take some inspiration from the PCI land to make our hwmod/omap_device a little easier from driver perspective. Or maybe it doesn't make sense ;-) This idea probably won't lead anywhere. To the best of my knowledge (and I'm not an expert on PCI), these different power wells are simply part of the PCI bus. The bus controller knows to turn off one of them when the device (or maybe when all the devices on a bus segment) goes into D3. One big difference with respect to OMAP is the way PCI handles wakeup messages. There's a separate bus signal, PME# (which stands for Power Management Event), that a device asserts when it wants to send a wakeup request. When the ACPI driver sees the PME# signal, it searches through the entire PCI tree to find the device or devices which need to be resumed. Or maybe when ACPI sees the signal, it tells the PCI core to do this -- I'm not sure of the details. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Tue, 2 Jul 2013, Roger Quadros wrote: On 07/02/2013 12:01 AM, Alan Stern wrote: On Mon, 1 Jul 2013, Felipe Balbi wrote: I don't know what Pad wakeup is. The wakeup signal has to originate from the EHCI controller, doesn't it? If not, how does the Pad know when a wakeup is needed? That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) inside and always on power domain. EHCI sits in another power domain which be turned off. When it's turned off (power gated and clock gated) the EHCI block has no means to wakeup whatsoever. That's when pad wakeup comes into play. It is generated when PRCM sees a change in the actual pad of the die. To check who should 'own' the wakeup, it checks the muxing settings to verify whose pad is that. How does the PRCM know which changes should generate wakeup events? It doesn't know. It will always generate a wakeup on any change in the monitored pins. We can only configure which pins we want to monitor. So we can't choose which wakeup events we want to monitor. We just can enable or disable all events. In the EHCI controller, this is managed by the settings of the WKOC_E, WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is powered off, those bits can't be used. Is powered off same as ehci_suspend? If yes then how does it work on other systems for remote wakeup? Above, Felipe wrote that on OMAP, EHCI sits in a power domain which is turned off: power gated and clock gated. ehci_suspend() doesn't do those things, but they are what I was referring to. A PCI-based EHCI controller has two power sources: the core well (which is turned off during suspend) and the auxiliary well (which remains powered). That's how remote wakeup works; it uses the auxiliary well. Also, once the wakeup signal has been turned on, how does it get turned off again? That is taken care of by the OMAP pinctrl driver. It will always turn off the wakeup signal once the event is detected and forwarded as an IRQ. I know that all this is a bit ugly :(. I still a little confused. The wakeup signal turns on. Then the pinctrl driver sees it, forwards it as an IRQ, and turns the wakeup signal off. But what if the IRQ is disabled (as it would be with your patch)? Does the IRQ line remain active until it causes an interrupt? What eventually turns off the IRQ line? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Mon, 1 Jul 2013, Roger Quadros wrote: On 06/28/2013 10:06 PM, Alan Stern wrote: On Fri, 28 Jun 2013, Roger Quadros wrote: That's not what I meant. Never mind the pinctrl; I was asking about the EHCI controller itself. Under what circumstances does the controller assert its wakeup signal? And how do you tell it to stop asserting that signal? I believe this would be through the EHCI Interrupt enable register (USBINTR). I'm not aware of any other mechanism. That's strange, because ehci_suspend() sets the intr_enable register to 0. So how do you ever get any wakeup interrupts at all? Because after ehci_suspend() for OMAP, we solely rely on the out of band wake up mechanism. i.e. Pad wakeup. I don't know what Pad wakeup is. The wakeup signal has to originate from the EHCI controller, doesn't it? If not, how does the Pad know when a wakeup is needed? We can't enable_irq at the start as the controller will only be resumed after usb_remote_wakeup(). Hmmm, yes, I had realized that at one point and then forgot it. You don't want an IRQ to arrive before you're prepared to handle it. This suggests that you really want to call enable_irq() call at the end of ehci_resume() instead of the beginning. (Convert the return 0 to a jump to the end of the routine.) Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Mon, 1 Jul 2013, Felipe Balbi wrote: I don't know what Pad wakeup is. The wakeup signal has to originate from the EHCI controller, doesn't it? If not, how does the Pad know when a wakeup is needed? That's really an OMAP thing, I guess. Pad wakeup sits in the PRCM (IIRC) inside and always on power domain. EHCI sits in another power domain which be turned off. When it's turned off (power gated and clock gated) the EHCI block has no means to wakeup whatsoever. That's when pad wakeup comes into play. It is generated when PRCM sees a change in the actual pad of the die. To check who should 'own' the wakeup, it checks the muxing settings to verify whose pad is that. How does the PRCM know which changes should generate wakeup events? In the EHCI controller, this is managed by the settings of the WKOC_E, WKDSCNNT_E, and WKCNNT_E bits in the PORTSC registers. But if EHCI is powered off, those bits can't be used. Also, once the wakeup signal has been turned on, how does it get turned off again? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Fri, 28 Jun 2013, Roger Quadros wrote: That's not what I meant. Never mind the pinctrl; I was asking about the EHCI controller itself. Under what circumstances does the controller assert its wakeup signal? And how do you tell it to stop asserting that signal? I believe this would be through the EHCI Interrupt enable register (USBINTR). I'm not aware of any other mechanism. That's strange, because ehci_suspend() sets the intr_enable register to 0. So how do you ever get any wakeup interrupts at all? Right. It seems the external hub has signaled remote wakeup but the kernel doesn't resume the root hub's port it is connected to. By observing the detailed logs below you can see that the root hub does not generate an INTerrupt transaction to notify the port status change event. I've captured the pstatus and GetPortStatus info as well. We don't need an interrupt. The driver is supposed to detect the remote wakeup sent by the external hub all by itself. Failing case [ 16.108032] usb usb1: usb auto-resume [ 16.108062] ehci-omap 48064800.ehci: resume root hub [ 16.108154] hub 1-0:1.0: hub_resume [ 16.108398] ehci_hub_control GetPortStatus, port 1 temp = 0x1000 [ 16.108459] ehci_hub_control GetPortStatus, port 2 temp = 0x14c5 Here's where we should detect it. Look at the GetPortStatus case in ehci_hub_control(); the PORT_RESUME bit (0x0040) is set in temp, so the Remote Wakeup received? code should run. In particular, these lines should run: /* resume signaling for 20 msec */ ehci-reset_done[wIndex] = jiffies + msecs_to_jiffies(20); usb_hcd_start_port_resume(hcd-self, wIndex); /* check the port again */ mod_timer(ehci_to_hcd(ehci)-rh_timer, ehci-reset_done[wIndex]); Therefore 20 ms later, around timestamp 16.128459, ehci_hub_status_data() should have been called. At that time, the root-hub port should have been fully resumed. [ 16.108551] hub 1-0:1.0: port 2: status 0507 change [ 16.108612] ehci_hub_control GetPortStatus, port 3 temp = 0x1000 [ 16.108642] hub 1-0:1.0: hub_activate submitting urb [ 16.109222] ehci_irq port 3 pstatus 0x1000 [ 16.109222] ehci_irq port 2 pstatus 0x14c5 [ 16.109252] ehci_irq port 1 pstatus 0x1000 [ 16.109374] hub 1-0:1.0: state 7 ports 3 chg evt But apparently nothing happened. Why not? Did the rh_timer get reset? Maybe you can find out what went wrong. (Hmmm, we seem to be missing a set_bit(wIndex, ehci-resuming_ports); line in there...) Also, why do you need omap-initialized? Do you think you might get a wakeup interrupt before the controller has been fully set up? I don't see how you could, given the pm_runtime_get_sync() call in the probe routine. During probe we need to runtime_resume the device before usb_add_hcd() since the controller clocks must be enabled before any registers are accessed. However, we cannot call ehci_resume() before usb_add_hcd(). So to prevent this chicken egg situation, I've used the omap-initialized flag. It only indicates that the ehci structures are initialized and we can call ehci_resume/suspend(). Ah, yes. Other subsystems, such as PCI, face exactly the same problem. You probably shouldn't call it initialized, though, because the same issue arises in ehci_hcd_omap_remove() -- the pm_runtime_put_sync() there would end up calling ehci_suspend() after usb_remove_hcd(). bound or started would be better names. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Fri, 28 Jun 2013, Roger Quadros wrote:
Just found the problem. It seems that enabling the ehci_irq _after_ the root
hub is resumed
is the root cause of the problem. Doing so will miss events from the root hub.
This sounds like a bug in the IRQ setup. It's the sort of thing you
see when a level-triggered IRQ is treated as though it were
edge-triggered.
In any case, the wakeup should have worked whether the IRQ was issued
or not.
The following modification worked for me without any delays.
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 72df4eb..b3af1aa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,11 +2136,6 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
- if (HCD_IRQ_DISABLED(hcd)) {
- /* Interrupt was disabled */
- clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
- enable_irq(hcd-irq);
- }
}
/**
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 593d28d..f2a1a46 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1110,6 +1110,12 @@ int ehci_resume(struct usb_hcd *hcd, bool hibernated)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+ if (HCD_IRQ_DISABLED(hcd)) {
+ /* Interrupt was disabled */
+ clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ enable_irq(hcd-irq);
+ }
+
if (time_before(jiffies, ehci-next_statechange))
msleep(100);
I appreciate the symmetry of putting the enable_irq call in ehci-hcd,
seeing as how the disable_irq is there too. On the other hand, every
HCD using this mechanism is going to have to do the same thing, which
argues for putting the enable call in the core. Perhaps at the start
of hcd_resume_work() instead of the end.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
don't see how you could, given the pm_runtime_get_sync() call in the probe routine. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Tue, 25 Jun 2013, Roger Quadros wrote:
On 06/24/2013 10:34 PM, Alan Stern wrote:
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
No and yes :). Actually the Pad wakeup comes as a separate IRQ from hardware.
The omap pinctrl driver captures that, determines which pad caused the wakeup
and
routes it to the appropriate interrupt based on the mapping provided in the
device tree.
In the ehci-omap case we provide the EHCI IRQ number in the mapping for the
USB host pads.
Could the mapping be changed so that a different interrupt vector was
used for wakeups and normal I/O? That would make this a little easier,
although it wouldn't solve the general problem.
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
There's still a race problem. Suppose a normal wakeup interrupt occurs
just before or as the controller gets suspended. By the time the code
here runs, HCD_HW_ACCESSIBLE may have been cleared by the suspend
routine. The interrupt would be lost. Depending on the design of the
controller, the entire wakeup signal could end up getting lost as well.
Do you know how the OMAP EHCI controller behaves? Under what
conditions does it send the wakeup IRQ? How do you tell it to turn off
the wakeup IRQ?
Looks good to me. Below is the implementation on these lines.
I've added a flag has_wakeup_irq:1 in struct hcd to indicate the special
case where the interrupt can come even if controller is suspended.
I've modified the ehci-omap driver to call ehci_suspend/resume() on system
suspend/resume. For runtime suspend/resume I just toggle the HCD_HW_ACCESSIBLE
flag to indicate that controller cannot be accessed when runtime suspended.
You're going to change that, right? ehci_suspend/resume should be
called whenever the controller's power state changes, regardless of
whether it is for runtime PM or system PM.
The cutting of clocks is done in the parent driver (i.e.
drivers/mfd/omap-usb-host.c)
Patch is below. If it looks OK. I'll re-post the entire series. Thanks.
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index f5f5c7d..51c2d59 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -110,6 +110,7 @@ struct usb_hcd {
#define HCD_FLAG_WAKEUP_PENDING 4 /* root hub is
resuming? */
#define HCD_FLAG_RH_RUNNING 5 /* root hub is running? */
#define HCD_FLAG_DEAD6 /* controller has died?
*/
+#define HCD_FLAG_IRQ_DISABLED7 /* Interrupt was
disabled */
/* The flags can be tested using these macros; they are likely to
* be slightly faster than test_bit().
@@ -120,6 +121,7 @@ struct usb_hcd {
#define HCD_WAKEUP_PENDING(hcd) ((hcd)-flags (1U
HCD_FLAG_WAKEUP_PENDING))
#define HCD_RH_RUNNING(hcd) ((hcd)-flags (1U HCD_FLAG_RH_RUNNING))
#define HCD_DEAD(hcd)((hcd)-flags (1U HCD_FLAG_DEAD))
+#define HCD_IRQ_DISABLED(hcd)((hcd)-flags (1U
HCD_FLAG_IRQ_DISABLED))
/* Flags that get set only during HCD registration or removal. */
unsignedrh_registered:1;/* is root hub registered? */
@@ -132,6 +134,7 @@ struct usb_hcd {
unsignedwireless:1; /* Wireless USB HCD */
unsignedauthorized_default:1;
unsignedhas_tt:1; /* Integrated TT in root hub */
+ unsignedhas_wakeup_irq:1; /* Can generate IRQ when
suspended */
unsigned intirq;/* irq allocated */
void __iomem*regs; /* device memory/io */
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..24e21a2
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Mon, 24 Jun 2013, Roger Quadros wrote:
OK I've tried to handle all this in an alternate way. Now the controller
suspend/resume
and runtime suspend/resume is independent of bus suspend.
The controller now runtime suspends when all devices on the bus have
suspended and
the hub auto suspends. NOTE: HW_ACCESSIBLE is still set on runtime_suspend.
The challenge here is to process the interrupt in this state.
The situation is a little peculiar. Does the hardware really use the
same IRQ for reporting wakeup events when the controller is suspended
and for reporting normal I/O events?
In principle, HW_ACCESSIBLE should not be set when the controller is
suspended, because you can't access the hardware then (since the clocks
are off, right?). But I can see how this would cause a problem if it
leads to wakeup interrupts being ignored.
Also, note that one of the things ehci_suspend() does is turn off the
Interrupt-Enable register, which means the wakeup interrupt would never
be issued in the first place. I guess ehci_hcd_omap_suspend() will
have to turn on the proper enable bit before stopping the clocks.
I've tried to handle this state. (i.e. interrupt while controller is runtime
suspended),
by disabling the interrupt till we are ready and calling
usb_hcd_resume_root_hub().
We mark a flag (HW_IRQ_DISABLED) stating that the interrupt was disabled and
based on
that enable the IRQ and clear the flag in hcd_resume_work().
Do let me know what you think of this approach.
This is a very tricky problem. Right now, usbcore assumes that when
HW_ACCESSIBLE is clear, the hardware can't generate interrupt requests
and therefore any interrupt must come from some other device sharing
the same IRQ line. For the systems you're working on, this is wrong in
both respects (the hardware _can_ generate interrupt requests and IRQ
lines aren't shared).
I think we will have to add a new flag to describe your situation.
Let's call it hcd-has_wakeup_interrupts. Presumably there will never
be a system that uses interrupts for wakeup signals _and_ has shared
IRQ lines? That would be a bad combination...
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..8879cd2 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2136,6 +2136,11 @@ static void hcd_resume_work(struct work_struct *work)
usb_lock_device(udev);
usb_remote_wakeup(udev);
usb_unlock_device(udev);
+ if (HCD_IRQ_DISABLED(hcd)) {
+ /* Interrupt was disabled */
+ clear_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ enable_irq(hcd-irq);
+ }
}
This part is okay.
@@ -2225,6 +2230,16 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd)))
rc = IRQ_NONE;
+ else if (pm_runtime_status_suspended(hcd-self.controller)) {
+ /*
+ * We can't handle it yet so disable IRQ, make note of it
+ * and resume root hub (i.e. controller as well)
+ */
+ disable_irq_nosync(hcd-irq);
+ set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
+ usb_hcd_resume_root_hub(hcd);
+ rc = IRQ_HANDLED;
+ }
This part will have to be different.
Certainly if HCD_DEAD(hcd) then we want to return IRQ_NONE. Likewise
if (!HCD_HW_ACCESSIBLE(hcd) !hcd-has_wakeup_interrupts). In all
other cases we have to call the HCD's interrupt handler.
The rest of the work will have to be done in the HCD, while holding the
private lock. In ehci_irq(), after the spin_lock() call, you'll have
to add something like this:
if (unlikely(!HCD_HW_ACCESSIBLE(hcd))) {
/*
* We got a wakeup interrupt while the controller was
* suspending or suspended. We can't handle it now, so
* disable the IRQ and resume the root hub (and hence
* the controller too).
*/
disable_irq_nosync(hcd-irq);
set_bit(HCD_FLAG_IRQ_DISABLED, hcd-flags);
spin_unlock(ehci-lock);
usb_hcd_resume_root_hub(hcd);
return IRQ_HANDLED;
}
I think this will work. How does it look to you?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/6] Suspend USB Host controller on bus suspend
On Thu, 20 Jun 2013, Roger Quadros wrote: As the omap-ehci controller driver needs to do some additional work to put itself into suspend/resume and make sure it is resumed to process an interrupt, we need to be able to override irq, bus_suspend, and bus_resume handlers. This provision is done in patch 3. Do you still need to override these things if you do the controller suspend at the right time? At least not for bus_suspend and bus_resume. We will still need to override the irq handler though. But, if we can take care of this generically in the ehci_irq handler (i.e. make sure controller is not suspended while accessing it) then we don't need such override for irq. I don't think you need to override anything. The high-level interrupt handler usb_hcd_irq() will avoid calling ehci_irq() when the HCD_HW_ACCESSIBLE flag isn't set. All you will need to do in ehci-omap.c is call synchronize_irq() after ehci_suspend() returns and before turning off the clocks. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/6] USB: ehci-omap: Suspend the controller during bus suspend
On Thu, 20 Jun 2013, Roger Quadros wrote:
runtime_resume(dev)
{
...
if (omap-flags OMAP_EHCI_IRQ_PENDING) {
process_pending_irqs(omap);
OK, thanks.
But I'm not sure if the generic ehci_irq handler is able to
run in a process context. Maybe if we replace spin_lock(ehci-lock);
with spin_lock_irqsave() there, it will work.
Alan is this a doable option?
ehci_irq() will work okay in process context, provided the caller
disables interrupts.
But maybe none of this will be needed after Roger redesigns the
controller suspend to work at the right time. Or if it is, we could
adopt a simpler alternative: the controller's resume routine could
always call usb_hcd_resume_root_hub(). After all, about the only
reason for doing a runtime resume of the controller is because the root
hub needs to do something.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/6] Suspend USB Host controller on bus suspend
On Wed, 19 Jun 2013, Roger Quadros wrote: Hi, This series attempts to suspend the OMAP EHCI host controller on USB Bus suspend. Why do you want to suspend the host controller during bus suspend? They are two different operations and should be carried out at two different times. The controller should be suspended during controller suspend, not during bus suspend. As the omap-ehci controller driver needs to do some additional work to put itself into suspend/resume and make sure it is resumed to process an interrupt, we need to be able to override irq, bus_suspend, and bus_resume handlers. This provision is done in patch 3. Do you still need to override these things if you do the controller suspend at the right time? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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] USB: ehci-omap: Reset dma_mask pointer on probe
On Thu, 23 May 2013, Roger Quadros wrote: Device tree probed devices don't get dma_mask set. Previously we were setting the dma_mask pointer only if it was NULL. However, the address of 'omap_ehci_dma_mask' would change each time the module is unloaded and loaded back thus causing the devices dma_mask pointer to be invalid on the next load. This will cause page faults if any driver tries to access the old dma_mask pointer. Unconditionally re-setting the dma_mask pointer fixes this problem. diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index 3d1491b..b33e306 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -146,8 +146,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev) * Since shared usb code relies on it, set it here for now. * Once we have dma capability bindings this can go away. */ - if (!pdev-dev.dma_mask) - pdev-dev.dma_mask = omap_ehci_dma_mask; + pdev-dev.dma_mask = omap_ehci_dma_mask; Is this the solution that people have agreed on? There has been a lot of discussion on this topic. In particular, there has been talk about fixing it in the DT core. This particular approach doesn't seem very robust. What if pdev-dev.dma_mask is already set to a different value for some good reason? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap 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: set device dma_mask without reference to global data
On Wed, 8 May 2013, Arnd Bergmann wrote: On Wednesday 08 May 2013, Greg Kroah-Hartman wrote: On Tue, May 07, 2013 at 04:53:52PM -0600, Stephen Warren wrote: From: Stephen Warren swar...@nvidia.com Suggested-by: Arnd Bergmann a...@arndb.de Signed-off-by: Stephen Warren swar...@nvidia.com So this needs to go in for 3.10, right? Any older kernels as well? If so, which ones? The fix should definitely go into 3.10, but I'd suggest waiting with the backport for a couple of -rc releases to avoid possible regressions. We know that the current code is broken, but few people fully understand what is going on with coherent_dma_mask, so it might cause new problems in combination with some other unknown bug, and I don't see this as urgent: none of the ARM defconfigs build this driver as a loadable module and there is no bug in the built-in case. For some reason, only the ARM back-end drivers are broken. The first occurence was apparently in 3.3, but only in ehci-tegra.c, while the other drivers subsequently copied the bug. An alternative solution -- perhaps not better but also not relying on coherent_dma_mask -- is to clear pdev-dev.dma_mask in the remove routine if it points to the static mask value. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Removing vestiges of CONFIG_USB_SUSPEND
Felipe and Kevin: While removing the remaining usages of USB_SUSPEND (things that I missed in the original patch), I noticed that arch/arm/configs/omap2plus_defconfig does not enable PM_RUNTIME -- even though it currently does enable USB_SUSPEND, which depends on PM_RUNTIME. Was this an oversight? In a little while I will submit a patch which removes USB_SUSPEND from that file. You or someone else may want to write a separate patch to enable PM_RUNTIME. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Tue, 23 Apr 2013, Vivek Gautam wrote: Alright, so here's my understanding: I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said that it could be done before that so that DWC3 sees an enabled PHY during probe. Basically right. Help me to understand the overall situation a little better: What code registers the PHY initially? PHY is added to global list by PHY drivers (like phy-samsung-usb2.c/phy-omap-usb2.c) by usb_add_phy() API Then this routine should initialize the PHY. The initialized state could be either active or suspended, your choice. Suspended would be best, in case the PHY never gets used. What routine does the DWC3 driver call to register itself as a consumer of the PHY? I think DWC3 doesn't registers itself as consumer of PHY, rather it gets that PHY from the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. DWC3 can now call PHY's initialization sequence using usb_phy_init(). So, before DWC3 initializes the PHY, PHYs should be in active state. Then usb_phy_init should make sure the PHY is in the active state. If usb_add_phy() left the PHY suspended, then this routine should call pm_runtime_get_sync(). After DWC3 (or any other driver) has acquired the PHY, it can call pm_runtime_put/get() however it likes, so long as the calls balance properly. If the driver isn't runtime-PM aware then it won't use any of these calls, and the PHY will remain active the entire time. Likewise, what routine does it call to unregister itself? Once DWC3's remove() is called PHYs are put. Is there a routine analogous to usb_phy_init() that gets called when PHY is released? That routine would do the opposite of usb_phy_init(), putting the PHY back into its initialized state. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Tue, 23 Apr 2013, Vivek Gautam wrote: Hi, On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 23 Apr 2013, Vivek Gautam wrote: Alright, so here's my understanding: I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said that it could be done before that so that DWC3 sees an enabled PHY during probe. Basically right. Help me to understand the overall situation a little better: What code registers the PHY initially? PHY is added to global list by PHY drivers (like phy-samsung-usb2.c/phy-omap-usb2.c) by usb_add_phy() API Then this routine should initialize the PHY. The initialized state could be either active or suspended, your choice. Suspended would be best, in case the PHY never gets used. Fair enough. What routine does the DWC3 driver call to register itself as a consumer of the PHY? I think DWC3 doesn't registers itself as consumer of PHY, rather it gets that PHY from the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. DWC3 can now call PHY's initialization sequence using usb_phy_init(). So, before DWC3 initializes the PHY, PHYs should be in active state. Then usb_phy_init should make sure the PHY is in the active state. If usb_add_phy() left the PHY suspended, then this routine should call pm_runtime_get_sync(). Right After DWC3 (or any other driver) has acquired the PHY, it can call pm_runtime_put/get() however it likes, so long as the calls balance properly. If the driver isn't runtime-PM aware then it won't use any of these calls, and the PHY will remain active the entire time. Alright, so DWC3 (or any other consumer of PHY) should do minimal to handle runtime state of PHYs; get() when accessing PHY and put() when it's done with it. Yes. The first operation will generally be a put, because usb_phy_init() will leave the PHY in an active state. Likewise, what routine does it call to unregister itself? Once DWC3's remove() is called PHYs are put. Is there a routine analogous to usb_phy_init() that gets called when PHY is released? That routine would do the opposite of usb_phy_init(), putting the PHY back into its initialized state. Yes, ofcourse there's a routine usb_phy_shutdown(). So this will be calling put_sync() to put PHYs back to its initialized state. Right ? Correct. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] USB: ehci-omap: Improve PHY error handling
On Wed, 17 Apr 2013, Roger Quadros wrote:
As the USB PHY layer never returns NULL we don't need
to check for that condition.
CC: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/usb/host/ehci-omap.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 5de3e43..3d1491b 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -175,12 +175,12 @@ static int ehci_hcd_omap_probe(struct platform_device
*pdev)
phy = devm_usb_get_phy_by_phandle(dev, phys, i);
else
phy = devm_usb_get_phy_dev(dev, i);
- if (IS_ERR(phy) || !phy) {
+ if (IS_ERR(phy)) {
/* Don't bail out if PHY is not absolutely necessary */
if (pdata-port_mode[i] != OMAP_EHCI_PORT_MODE_PHY)
continue;
- ret = IS_ERR(phy) ? PTR_ERR(phy) : -ENODEV;
+ ret = PTR_ERR(phy);
dev_err(dev, Can't get PHY device for port %d: %d\n,
i, ret);
goto err_phy;
Acked-by: Alan Stern st...@rowland.harvard.edu
--
To unsubscribe from this list: send the line unsubscribe linux-omap 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/2] USB: ehci-omap: Don't select any PHY driver
On Mon, 15 Apr 2013, Roger Quadros wrote: Don't select NOP_USB_XCEIV. Instead, board config must select USB_PHY and the appropriate PHY driver. Also add a hint in Kconfig so that users enabling this driver manually enable the right PHY drivers as well. Gets rid of the below warnings when USB_EHCI_HCD_OMAP is enabled. warning: (USB_EHCI_HCD_OMAP) selects NOP_USB_XCEIV which has unmet direct dependencies (USB_SUPPORT USB_PHY) warning: (USB_EHCI_HCD_OMAP) selects NOP_USB_XCEIV which has unmet direct dependencies (USB_SUPPORT USB_PHY) CC: Alan Stern st...@rowland.harvard.edu Signed-off-by: Roger Quadros rog...@ti.com --- drivers/usb/host/Kconfig |4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index c0be25c..c558472 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -150,11 +150,13 @@ config USB_EHCI_MXC config USB_EHCI_HCD_OMAP tristate EHCI support for OMAP3 and later chips depends on ARCH_OMAP - select NOP_USB_XCEIV default y ---help--- Enables support for the on-chip EHCI controller on OMAP3 and later chips. + If your system uses a PHY on the USB port, you will need to + enable USB_PHY and the appropriate PHY driver as well. Most + boards need the NOP_USB_XCEIV PHY driver. config USB_EHCI_HCD_ORION tristate Support for Marvell EBU on-chip EHCI USB controller Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] USB: ehci-omap: Improve PHY error handling
On Mon, 15 Apr 2013, Roger Quadros wrote:
As the USB PHY layer never returns NULL we don't need
to check for that condition.
If we fail to get the PHY device it could be due
to missing USB PHY drivers. Give this hint to the user
in the error message.
CC: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Roger Quadros rog...@ti.com
---
drivers/usb/host/ehci-omap.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 5de3e43..2e34ddd 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -175,13 +175,13 @@ static int ehci_hcd_omap_probe(struct platform_device
*pdev)
phy = devm_usb_get_phy_by_phandle(dev, phys, i);
else
phy = devm_usb_get_phy_dev(dev, i);
- if (IS_ERR(phy) || !phy) {
+ if (IS_ERR(phy)) {
/* Don't bail out if PHY is not absolutely necessary */
if (pdata-port_mode[i] != OMAP_EHCI_PORT_MODE_PHY)
continue;
- ret = IS_ERR(phy) ? PTR_ERR(phy) : -ENODEV;
- dev_err(dev, Can't get PHY device for port %d: %d\n,
+ ret = PTR_ERR(phy);
+ dev_err(dev, Can't get PHY device for port %d: %d. Is
USB_PHY driver enabled?\n,
i, ret);
goto err_phy;
}
Getting rid of the !phy test is good. But I'm doubtful about the
change to the error message. Are you going to make a similar change to
every platform driver? There doesn't seem to be any reason to do this
for ehci-omap but not the others.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Thu, 4 Apr 2013, Felipe Balbi wrote: Some subsystems handle this issue by calling pm_runtime_get_sync() before probing a driver and pm_runtime_put_sync() after unbinding the driver. If the driver is runtime-PM-enabled, it then does its own put_sync near the end of its probe routine and get_sync in its release routine. sounds a bit 'fishy' to me... So a separate entity would call pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, I don't know what you mean by separate entity. The PHY's subsystem would handle this. After all, the subsystem has to handle registering the PHY in the first place. If the PHY doesn't have a dev_pm_ops, why are you talking about doing runtime PM on it? That doesn't make any sense. then drivers need to check if runtime_pm is enabled and call pm_runtime_put*() conditionally before returning from probe(). One They don't have to check. If CONFIG_PM_RUNTIME isn't set or the target is runtime-PM-disabled then pm_runtime_put* is essentially a no-op (in the disabled case it decrements the usage counter but doesn't do anything else). One possible complication I did not mention: The scheme described above assumes the device that uses the PHY will always be registered on the same type of bus. If the device can be used on multiple bus types (and hence in multiple subsystems) then things aren't so simple, because some of the subsystems might support runtime PM and others might not. (You may very well run into this problem with USB controllers that are sometimes on a PCI bus and sometimes on a platform bus.) In this case, the driver needs to adapt to the subsystem's capabilities. Presumably the bus-glue part of the driver takes care of this. remove, we might have another issue: device is already runtime_suspended (due to e.g. autosuspend) when module is removed, a call to pm_runtime_put_sync() will be unbalanced. No ? No. I left out some of the details. For one thing, the subsystem is careful to do a runtime resume before calling the driver's remove method. (Also, if you look over my original description carefully, you'll see that there are no unbalanced calls -- even if the device is already runtime suspended when the driver is unbound.) For another, the subsystem needs to check before calling the driver's runtime-PM methods. There are two brief windows during which the driver isn't in charge of the device even though dev-driver is set. Those windows occur in the subsystem's probe routine (before it calls the driver's probe method) and in the subsystem's remove routine (after it calls the driver's remove method). At such times, the subsystem's runtime-PM handlers must be careful _not_ to call the driver's runtime-PM routines. May be i am misinterpreting !! If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*), then the consumers need to call pm_runtime_get_sync whever they want to access PHY. No, because in addition to being runtime-PM enabled, the PHY should automatically be runtime resumed when the consumer registers itself as a user of the PHY. Therefore the consumer doesn't need to do anything at all -- which is good for consumers that aren't runtime-PM aware. Alright, so here's my understanding: I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said that it could be done before that so that DWC3 sees an enabled PHY during probe. Basically right. Help me to understand the overall situation a little better: What code registers the PHY initially? What routine does the DWC3 driver call to register itself as a consumer of the PHY? Likewise, what routine does it call to unregister itself? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Wed, 3 Apr 2013, Felipe Balbi wrote: Lets suppose DWC3 enables runtime_pm on USB 2 type phy, it will try to go into suspend state and thereby call runtime_suspend(), if any. And PHY will come to active state only when its consumer wakes it up, and this consumer is operational only when its related PHY is in fully functional state. So do we have a situation in which this PHY goes into low power state in its runtime_suspend(), resulting in non-detection of devices on further attach (since PHY is in low power state) ? Will the controller (like EHCI/OHCI) be functional now ? ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), right ? (so does DWC3 :-) Maybe you guys have already got this all figured out -- if so, feel free to ignore this email. Some subsystems handle this issue by calling pm_runtime_get_sync() before probing a driver and pm_runtime_put_sync() after unbinding the driver. If the driver is runtime-PM-enabled, it then does its own put_sync near the end of its probe routine and get_sync in its release routine. With PHYs you don't have probing and releasing, but you do have consumers registering and unregistering themselves, which is about the same thing. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] bisected: PandaBoard smsc95xx ethernet driver error from USB timeout
On Wed, 20 Mar 2013, Frank Rowand wrote: Hi All, Not quite sure quite where the problem is (USB, OMAP, smsc95xx driver, other???), so casting the nets wide... The PandaBoard frequently fails to boot with an eth0 error when mounting the root file system via NFS (ethernet driver fails due to a USB timeout; no ethernet means NFS won't work). A typical set of error messages is: [3.264373] smsc95xx 1-1.1:1.0: usb_probe_interface [3.269500] smsc95xx 1-1.1:1.0: usb_probe_interface - got id [3.275543] smsc95xx v1.0.4 [8.078674] smsc95xx 1-1.1:1.0: eth0: register 'smsc95xx' at usb-ehci-omap.0-1.1, smsc95xx USB 2.0 Ethernet, 82:b9:1d:fa:67:0d [8.091003] hub 1-1:1.0: state 7 ports 5 chg evt 0002 [ 13.509918] usb 1-1.1: swapper/0 timed out on ep0out len=0/4 [ 13.515869] smsc95xx 1-1.1:1.0: eth0: Failed to write register index 0x0108 [ 13.523559] smsc95xx 1-1.1:1.0: eth0: Failed to write ADDRL: -110 [ 13.529998] IP-Config: Failed to open eth0 I have bisected this to: commit 18aafe64d75d0e27dae206cacf4171e4e485d285 Author: Alan Stern st...@rowland.harvard.edu Date: Wed Jul 11 11:23:04 2012 -0400 USB: EHCI: use hrtimer for the I/O watchdog I don't understand how that commit could cause a timeout unless there are at least two other bugs present in your system. Note that to compile this version of the kernel, an additional fix must also be applied: commit ba5952e0711b14d8d4fe172671f8aa6091ace3ee Author: Ming Lei ming@canonical.com Date: Fri Jul 13 17:25:24 2012 +0800 USB: ehci-omap: fix compile failure(v1) The symptom can be worked around by retrying the USB access if a timeout occurs. This is clearly _not_ the fix, just a hack that I used to investigate the problem: http://article.gmane.org/gmane.linux.rt.user/9773 My kernel configuration is: arch/arm/configs/omap2plus_defconfig plus to get the ethernet driver I add: CONFIG_USB_EHCI_HCD CONFIG_USB_NET_SMSC95XX I found the problem on 3.6.11, but have not replicated it on 3.9-rcX yet because my config fails to build on 3.9-rc1 and 3.9-rc2. I'll try to work on that issue tomorrow. Let me know how it works out. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] USB: ehci-omap: Try to get PHY even if not in PHY mode
On Thu, 14 Mar 2013, Felipe Balbi wrote:
if (IS_ERR(phy) || !phy) {
+ /* Don't bail out if PHY is not absolutely necessary */
+ if (pdata-port_mode[i] != OMAP_EHCI_PORT_MODE_PHY)
+ continue;
+
ret = IS_ERR(phy) ? PTR_ERR(phy) : -ENODEV;
dev_err(dev, Can't get PHY device for port %d: %d\n,
i, ret);
Felipe, this is a strange interface. Why do we sometimes get an
error-pointer and sometimes get just NULL? Why not always an
error-pointer?
looks like we get NULL when PHY layer is disabled. Sounds like an
oversight to me. Do you want to send a patch, or do I cook one and put
yourself as Reported-by ?
You're more familiar with that code. Reported-by is good enough for
me. :-)
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 12/13] USB: ehci-omap: Fix detection in HSIC mode
On Wed, 13 Mar 2013, Roger Quadros wrote: Move PHY initialization until after EHCI initialization is complete, instead of initializing the PHYs first, shutting them down again, and then initializing them a second time. This fixes HSIC device detection. Signed-off-by: Roger Quadros rog...@ti.com CC: Alan Stern st...@rowland.harvard.edu Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/13] USB: ehci-omap: Get rid of omap_ehci_init()
On Wed, 13 Mar 2013, Roger Quadros wrote: As it does almost nothing, get rid of omap_ehci_init() and move the ehci-caps initialization part into probe(). Also remove the outdated TODO list from header. Signed-off-by: Roger Quadros rog...@ti.com CC: Alan Stern st...@rowland.harvard.edu Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] USB: ehci-omap: Try to get PHY even if not in PHY mode
On Tue, 12 Mar 2013, Roger Quadros wrote:
Even when not in PHY mode, the USB device on the port (e.g. HUB)
might need resources like RESET which can be modelled as a PHY
device. So try to get the PHY device in any case.
Signed-off-by: Roger Quadros rog...@ti.com
CC: Alan Stern st...@rowland.harvard.edu
Acked-by: Alan Stern st...@rowland.harvard.edu
/* get the PHY device */
if (dev-of_node)
phy = devm_usb_get_phy_by_phandle(dev, phys, i);
else
phy = devm_usb_get_phy_dev(dev, i);
if (IS_ERR(phy) || !phy) {
+ /* Don't bail out if PHY is not absolutely necessary */
+ if (pdata-port_mode[i] != OMAP_EHCI_PORT_MODE_PHY)
+ continue;
+
ret = IS_ERR(phy) ? PTR_ERR(phy) : -ENODEV;
dev_err(dev, Can't get PHY device for port %d: %d\n,
i, ret);
Felipe, this is a strange interface. Why do we sometimes get an
error-pointer and sometimes get just NULL? Why not always an
error-pointer?
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/12] USB: ehci-omap: Fix detection in HSIC mode
On Tue, 12 Mar 2013, Roger Quadros wrote:
The HSIC devices need to be kept in reset while the EHCI controller
is being initialized and only brought out of reset after the
initialization is complete, else HSIC devices will not be detected.
This is not a great description of what the patch does. You should say
that it moves PHY initialization until after EHCI initialization is
complete, instead of initializing the PHYs first, shutting them down
again, and then initializing them a second time.
Also remove outdated TODO list from header.
Signed-off-by: Roger Quadros rog...@ti.com
CC: Alan Stern st...@rowland.harvard.edu
---
drivers/usb/host/ehci-omap.c | 38 +++---
1 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 1ba1df8..dc2de47 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -29,12 +29,6 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*
- * TODO (last updated Feb 27, 2010):
- * - add kernel-doc
- * - enable AUTOIDLE
- * - add suspend/resume
- * - add HSIC and TLL support
- * - convert to use hwmod and runtime PM
*/
#include linux/kernel.h
@@ -90,26 +84,13 @@ static inline u32 ehci_read(void __iomem *base, u32 reg)
static int omap_ehci_init(struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
- struct omap_hcd *omap = (struct omap_hcd *)ehci-priv;
- int rc, i;
-
- /* Hold PHYs in reset while initializing EHCI controller */
- for (i = 0; i omap-nports; i++) {
- if (omap-phy[i])
- usb_phy_shutdown(omap-phy[i]);
- }
+ int rc;
/* we know this is the memory we want, no need to ioremap again */
ehci-caps = hcd-regs;
rc = ehci_setup(hcd);
- /* Bring PHYs out of reset */
- for (i = 0; i omap-nports; i++) {
- if (omap-phy[i])
- usb_phy_init(omap-phy[i]);
- }
-
return rc;
}
Now that there's almost nothing left of this function, you can get rid
of it completely. Move the assignment to ehci-caps into
ehci_hcd_omap_probe() and remove the init entry in the overrides
structure.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/10] usb: phy: cleanups to Kconfig and directories
On Thu, 7 Mar 2013, Felipe Balbi wrote: Hi folks, inspired by Paul's DWC2 patchset which added usb_otg_state_string() (a copy of otg_state_string()) I have now renamed otg_state_string() to usb_otg_state_string(), moved it to usb-common, then moved all phy drivers to drivers/usb/phy/ and completely deleted the otg directory. We're also removing CONFIG_USB_OTG_UTILS since that has lots its meaning long ago. I have compiled all patches with allyes, allno and allmod configs, but please make sure to test on your platforms to make sure we're not leaking any more problems to mainline. Acked-by: Alan Stern st...@rowland.harvard.edu for the EHCI changes. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat
On Mon, 4 Mar 2013, Vivek Gautam wrote: @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev) if (ret) goto put_usb3_hcd; + pm_runtime_enable(pdev-dev); This is generally not a good idea. You shouldn't enable a device for runtime PM without first telling the PM core what state it is in. Right, but i am not completely sure on any fixed path to follow for enabling runtime power management on a device. :-( Does it become necessary to pm_runtime_set_active or pm_runtime_set_suspended a device before pm_runtime_enable ? Yes, it usually is necesary. And especially here, because the runtime PM core sets the initial status of every device to RPM_SUSPENDED. pm_runtime_enable would just try to decrement the disable_depth of the device. That's right. And once that happens, the PM core would think the device was suspended even though it was really at full power. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat
On Sun, 3 Mar 2013, Felipe Balbi wrote: this is good point and, in fact, a doubt I have myself. How are we supposed to check if device is suspended ? In case it _is_ suspended we might not be able to read device's registers due to clocks possibly being gated. That's really a separate question. It has a simple answer, though: If you want to access a device's registers, call pm_runtime_get_sync() beforehand and pm_runtime_put() (or _put_sync()) afterward. Then it won't matter if the device was suspended originally. that's alright, but how do you want me to check if my device is enabled or not before pm_runtime_enable() ? You're not supposed to check. Just make sure your own pm_runtime_enable() and _disable() calls are done correctly, and let the PM core worry about the rest. :-) More to the point, the question of what code enables a device for runtime PM is decided by the subsystem. Many subsystems will do it automatically so that their drivers don't have to worry about it. Other subsystems will leave it entirely up to the drivers. You have to know what the subsystem wants. In this case, it's appropriate to do the enable here in the probe routine because the platform core doesn't do it for you. This also means the driver should disable the device in the remove routine. I don't know much about clock handling. In general, the pm_runtime_set_active() and pm_runtime_enable() parts should be done by the subsystem, not the driver, whenever possible. good to know :-) Though I disagree with calling pm_runtime_enable() at the subsystem level. It depends on the subsystem. For some (like the USB host subsystem), it is appropriate. Whichever piece of code is responsible for associating a clock with a device should also be responsible for making sure that neither is suspended when the driver's probe routine runs. I'm not sure how different platforms do this; using a PM domain could be one answer. that's alright, but it generates a different set of problems. That same piece of code which associates a device with its clock, doesn't really know if the driver is even available which means we could be enabling clocks for no reason and just wasting precious battery juice ;-) Better than wasting our precious bodily fluids... :-) I guess the best answer is to set up the association but then leave the device and clocks in a runtime-suspended status. Then do a pm_runtime_get_sync() just before calling the driver's probe routine and a pm_runtime_put_sync() just after calling the remove routine. That should leave the device and the clocks in the state the driver expects. (But be careful that these two calls don't invoke the driver's runtime-PM callbacks!) Probably nobody has thought these problems through very carefully for the platform subsystem. Nevertheless, that's where the decisions need to be made. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat
On Sat, 2 Mar 2013, Vivek Gautam wrote: By enabling runtime pm in this driver allows users of xhci-plat to enter into runtime pm. This is not full runtime pm support (AKA xhci-plat doesn't actually power anything off when in runtime suspend mode) but, just basic enablement. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com CC: Doug Anderson diand...@chromium.org --- drivers/usb/host/xhci-plat.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index c9c7e13..595cb52 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -12,6 +12,7 @@ */ #include linux/platform_device.h +#include linux/pm_runtime.h #include linux/module.h #include linux/slab.h @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev) if (ret) goto put_usb3_hcd; + pm_runtime_enable(pdev-dev); This is generally not a good idea. You shouldn't enable a device for runtime PM without first telling the PM core what state it is in. @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev) struct usb_hcd *hcd = platform_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + if (!pm_runtime_suspended(dev-dev)) + pm_runtime_put(dev-dev); + pm_runtime_disable(dev-dev); + usb_remove_hcd(xhci-shared_hcd); usb_put_hcd(xhci-shared_hcd); This is very strange. Why have a pm_runtime_put with no balancing pm_runtime_get? And why use an async routine when you're about to unbind the driver? Don't you want the callback to occur before the unbinding? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat
On Sat, 2 Mar 2013, Felipe Balbi wrote: @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev) struct usb_hcd *hcd = platform_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + if (!pm_runtime_suspended(dev-dev)) + pm_runtime_put(dev-dev); + pm_runtime_disable(dev-dev); + usb_remove_hcd(xhci-shared_hcd); usb_put_hcd(xhci-shared_hcd); This is very strange. Why have a pm_runtime_put with no balancing pm_runtime_get? this is good point and, in fact, a doubt I have myself. How are we supposed to check if device is suspended ? In case it _is_ suspended we might not be able to read device's registers due to clocks possibly being gated. That's really a separate question. It has a simple answer, though: If you want to access a device's registers, call pm_runtime_get_sync() beforehand and pm_runtime_put() (or _put_sync()) afterward. Then it won't matter if the device was suspended originally. If you actually do want to tell whether or not a device is suspended and nothing more, call pm_runtime_status_suspended(). Of course, this is racy -- the power state might change right after you make the call. Also, considering that some drivers are used in multiple platforms and those might behave differently when it comes to clock handling, how do we do that ? Should we require drivers to explicitly clk_get(); clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ? I don't know much about clock handling. In general, the pm_runtime_set_active() and pm_runtime_enable() parts should be done by the subsystem, not the driver, whenever possible. While that's doable, I don't see how that'd be doable for OMAP since they want to hide clock handling from drivers. Any tips ? Whichever piece of code is responsible for associating a clock with a device should also be responsible for making sure that neither is suspended when the driver's probe routine runs. I'm not sure how different platforms do this; using a PM domain could be one answer. All this is somewhat off the point of my original comment, however. Drivers must be sure to balance their pm_runtime_get() and _put() calls. Having an unbalanced _put() in the remove routine is almost certainly a mistake -- especially if it is conditional on the device's power state, because a device can remain unsuspended even after the driver does a pm_runtime_put(). For example, this will happen if the user wrote on to /sys/.../power/control. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MUSB regression in linux next at least for pandboard
On Wed, 6 Feb 2013, Alan Stern wrote: Is this issue fixed ? Actually we too are getting very similar issue with samsung exynos5250 hardware. With latest 'usb-next' kernel and supporting arch patches, when i use following test scenerio: Connect a USB 2.0 external hub to USB 2.0 port, and connect mice or keyboard enumeration and functionality is fine but once disconnecting the HID we get to see the error log: hid-generic 0003:04B3:3025.0002: can't reset device, s5p-ehci-1.3/input0, status -71 When i tried to enable CONFIG_USB_DEBUG, get the following log: looks like it's not OMAP-specific. Alan any tips ? It could be a problem with the hub the keyboard is plugged into. Or something going on with the hub driver. I'll try doing the same thing on my system. I tried it. This is on an Intel system, not OMAP or anything like that. The result was as expected; there were a few can't reset device error messages but they stopped after a few hundred ms. If they persist for several minutes then something else is wrong. On the other hand, we could change the priority of those log messages. They don't have to be errors or warnings. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/14] USB: ehci-omap: Fix autoloading of module
On Thu, 7 Feb 2013, Roger Quadros wrote: The module alias should be ehci-omap and not omap-ehci to match the platform device name. The omap-ehci module should now autoload correctly. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Alan Stern st...@rowland.harvard.edu --- drivers/usb/host/ehci-omap.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index a8ce10d..a2d16c0 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -340,9 +340,10 @@ static void __exit ehci_omap_cleanup(void) } module_exit(ehci_omap_cleanup); -MODULE_ALIAS(platform:omap-ehci); +MODULE_ALIAS(platform:ehci-omap); MODULE_AUTHOR(Texas Instruments, Inc.); MODULE_AUTHOR(Felipe Balbi felipe.ba...@nokia.com); +MODULE_AUTHOR(Roger Quadros rog...@ti.com); MODULE_DESCRIPTION(DRIVER_DESC); MODULE_LICENSE(GPL); -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MUSB regression in linux next at least for pandboard
On Wed, 6 Feb 2013, Felipe Balbi wrote:
I can't reproduce the problem on Panda, but I can on Beagle with a slightly
different behaviour.
1) connecting/disconnecting device directly to the beagleboard's USB port
works fine.
2) If I connect a USB Hub to the port and connect a device to it after the
hub has
autosuspended, the device is never detected.
doing lsusb after that triggers the detection and produces a lot of
transaction errors.
Beagle log is below, before and after 'lsusb'
I suppose this doesn't affect Panda because it has a hub connected
immediately below the
root hub that never suspends (as ethernet is hardwired to it).
Roger, try changing hub's autosuspend delay to something greater than
30ms and see if it helps. There was a discussion lately about that.
There also were some patches to address this problem recently merged by
Greg KH (they are in Linus's current git, added after 3.8-rc6 was
released):
da0aa7169b97d90f4af39a9dc84d58bbe19d7e78 USB: add
usb_hcd_{start,end}_port_resume
f292e7f9fb0e4bec68bbd83443407d6bb7922d36 USB: EHCI: notify usbcore about port
resumes
ee74290b7853db9d5fd64db70e5c175241c59fba USB: EHCI: fix timer bug affecting
port resume
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: MUSB regression in linux next at least for pandboard
On Wed, 6 Feb 2013, Felipe Balbi wrote: Hi, On Wed, Feb 06, 2013 at 05:27:52PM +0530, Vivek Gautam wrote: Hi Tony, On Fri, Oct 5, 2012 at 9:57 PM, Tony Lindgren t...@atomide.com wrote: * Tony Lindgren t...@atomide.com [121004 18:41]: Also on the EHCI port, I've seen issues where unplugging the cable hangs kernel with an infinite loop. But that happens only occasionally, sorry does not seem to happen right now so no output to paste here. Or maybe this issue has already been fixed? Looks like the system eventually recovers from the EHCI issue after about fivew minutes or so of spamming the logs. It seems the ehci-omap errors are: [62934.201538] ehci-omap ehci-omap.0: detected XactErr len 0/8 retry 31 [62934.201660] ehci-omap ehci-omap.0: devpath 1.3.7 ep0out 3strikes [62934.201873] ehci-omap ehci-omap.0: reused qh ea5632c0 schedule More data below. Is this issue fixed ? Actually we too are getting very similar issue with samsung exynos5250 hardware. With latest 'usb-next' kernel and supporting arch patches, when i use following test scenerio: Connect a USB 2.0 external hub to USB 2.0 port, and connect mice or keyboard enumeration and functionality is fine but once disconnecting the HID we get to see the error log: hid-generic 0003:04B3:3025.0002: can't reset device, s5p-ehci-1.3/input0, status -71 When i tried to enable CONFIG_USB_DEBUG, get the following log: looks like it's not OMAP-specific. Alan any tips ? It could be a problem with the hub the keyboard is plugged into. Or something going on with the hub driver. I'll try doing the same thing on my system. (keeping logs below) ... s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 1 usb 1-1.3: unlink qh8-0e01/c193f140 start 2 [1/2 us] s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 1 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 2 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 3 hub 1-1:1.0: state 7 ports 7 chg evt 0008 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 4 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 5 hub 1-1:1.0: port 3, status 0100, change 0001, 12 Mb/s s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 6 usb 1-1.3: USB disconnect, device number 5 usb 1-1.3: unregistering device usb 1-1.3: unregistering interface 1-1.3:1.0 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 7 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 8 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 9 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 10 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 11 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 12 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 13 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 14 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 15 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 16 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 17 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 18 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 19 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 20 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 21 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 22 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 23 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 24 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 25 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 26 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 27 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 28 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 29 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 30 s5p-ehci s5p-ehci: detected XactErr len 0/8 retry 31 s5p-ehci s5p-ehci: devpath 1.3 ep0out 3strikes usb 1-1: clear tt buffer port 3, a5 ep0 t00080248 hid-generic 0003:04B3:3025.0002: can't reset device, s5p-ehci-1.3/input0, status -71 Note that most of these are debug messages, so they wouldn't normally appear. (BTW: timestamps would be nice -- CONFIG_PRINTK_TIME.) Similar log as you get on ehci-omap ;-) Sorry i might have missed some information to put here. Your help will be very much useful. Thanks in advance :-) ... [62927.200012] ehci-omap ehci-omap.0: detected XactErr len 0/8 retry 19 [62927.215606] ehci-omap ehci-omap.0: detected XactErr len 0/8 retry 25 [62927.220092] ehci-omap ehci-omap.0: detected XactErr len 0/8 retry 22 [62927.225738] ehci-omap ehci-omap.0: devpath 1.3.7 ep0out 3strikes A certain amount of this is normal when an HID device is unplugged. It should stop after 250 ms, however, when the hub notifies the host that a cable has been unplugged. (Unless the hub driver is busy doing something else at the time...) ... After waiting for several minutes, it stops, and dmesg shows: # dmesg | grep -i omap This grep will drop the important information. Alan Stern -- To unsubscribe from
Re: [PATCH 05/13] USB: ehci-omap: Get platform resources by index rather than by name
On Mon, 4 Feb 2013, Roger Quadros wrote: Since there is only one resource per type we don't really need to use resource name to obtain it. This also also makes it easier for device tree adaptation. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/13] USB: ohci-omap3: Get platform resources by index rather than by name
On Mon, 4 Feb 2013, Roger Quadros wrote: Since there is only one resource per type we don't really need to use resource name to obtain it. This also also makes it easier for device tree adaptation. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/13] USB: ohci-omap3: Add device tree support and binding information
On Mon, 4 Feb 2013, Roger Quadros wrote: Allows the OHCI controller found in OMAP3 and later chips to be specified via device tree. Signed-off-by: Roger Quadros rog...@ti.com For the ohci-omap3 part: Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/13] USB: ehci-omap: Add device tree support and binding information
On Mon, 4 Feb 2013, Roger Quadros wrote: Allows the OMAP EHCI controller to be specified via device tree. Signed-off-by: Roger Quadros rog...@ti.com For the ehci-omap part: Acked-by: Alan Stern st...@rowland.harvard.edu -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices
On Thu, 6 Dec 2012, Andy Green wrote:
Right. The question is should it be done in this somewhat roundabout
way (you've got two separate assets for setting up this one thing), or
should the USB port code be responsible for explicitly checking if
there are any applicable assets when the port is created?
The advantange of the second approach is that it doesn't involve
checking all the ancestors every time a new device is added (and it
involves only one asset). The disadvantage is that it works only for
USB ports.
It's done in two steps to strongly filter candidate child devices
against being children of a known platform device. If you go around
that, you will be back to full device path matching with wildcards at
the USB child to determine if he is the one. So that's a feature not
an issue I think.
I don't follow. Suppose an asset is attached to ehci-omap.0 with its
string set to -0:1.0/port1 and the other members pointing to the
regulator, clock and so on. And suppose the USB port code, when
creating a new port, checks for a name match against all the assets
attached to its bus's host controller device structure. That should
do exactly what you want while using only one asset.
I can see doing it generically or not is equally a political issue as a
technical one, but I think if we bother to add this kind of support we
should prefer to do it generally. It's going to be about the same
amount of code.
Not quite. If you do it generally then you have to insert hooks at all
the places where a subsystem _might_ need it. If you do it
specifically then no hooks are needed at all -- just a match call at
the right place in the subsystem that needs it.
As Tony Lindgren said, even board-omap4panda.c itself is deprecated, to
project platform info into USB stack you either have to add DT code into
usb stack then to go find things directly, or provide a generic
methodology like assets which have the dt bindings done outside of any
subsystem and apply their operations outside the subsystem too.
Assuming DT can be extended to support assets, adding one asset will be
simpler than adding two.
To answer the question, we need to know how other subsystems might
want to use the asset-matching approach. My guess is that only a small
number of device types would care about assets (in the same way that
assets matter to USB ports but not to other USB device types). And it
might not be necessary to check against every ancestor; we might know
beforehand where to check (just as the USB port would know to look for
an asset attached to the host controller device).
Yes I think it boils down to buses in general can benefit from this.
They're the thing that dynamically - later - create child devices you
might need to target with what was ultimately platform information.
On Panda the other bus thing that can benefit is the WLAN module on
SDIO. In fact that's a very illuminating case for your question above.
Faced with exactly the same problem, that they needed to project
platform info on to SDIO-probed device instance to tell it what clock
rate it had been given, they invented an api which you can see today in
board-omap4panda.c and other boards there, wl12xx_set_platform_data().
This is from board-4430sdp:
static struct wl12xx_platform_data omap4_sdp4430_wlan_data __initdata = {
.board_ref_clock = WL12XX_REFCLOCK_26,
.board_tcxo_clock = WL12XX_TCXOCLOCK_26,
};
...
ret = wl12xx_set_platform_data(omap4_sdp4430_wlan_data);
You can find the other end of it here in
drivers/net/wireless/ti/wlcore/wl12xx_platform_data.c
static struct wl12xx_platform_data *platform_data;
int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
{
if (platform_data)
return -EBUSY;
if (!data)
return -EINVAL;
platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
if (!platform_data)
return -ENOMEM;
return 0;
}
when the driver for the device instance wants to get its platform data
it reads the static pointer via another api. Obviously if you want two
modules on your platform that's not so good.
Also it isn't type-safe, although that's probably not a big concern.
I doubt that's the only SDIO device that wants to know platform info.
So I think by providing a generic way we help other buses and devices.
I agree, assets look like a good way to improve this. In fact, to some
extent assets appear to be a generalization or extension of platform
data.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices
On Wed, 5 Dec 2012, Andy Green wrote:
The details of this aren't clear yet. For instance, should the device
core try to match the port with the asset info, or should this be done
by the USB code when the port is created?
Currently what I have (this is before changing it to pm domain, but this
should be unchanged) lets the asset define a callback op which will
receive notifications for all added child devices that have the device
the asset is bound to as an ancestor.
So you would bind an asset to the host controller device...
static struct device_asset assets_ehci_omap0[] = {
{
.name = -0:1.0/port1,
.asset = assets_ehci_omap0_smsc_port,
.ops = device_descendant_attach_default_asset_ops,
},
{ }
};
...with this descendant filter callback pointing to a generic end of
the device path matcher.
when device_descendant_attach_default_asset_ops() sees the child that
was foretold has appeared (and it only hears about children of
ehci-omap.0 in this case), it binds the assets pointed to by .asset to
the new child before its probe. assets_ehci_omap0_smsc_port is an array
of the actual regulator and clock that need switching by the child. So
the effect is to magic the right assets to the child device just before
it gets added (and probe called etc).
This is working well and the matcher helper is small and simple.
Right. The question is should it be done in this somewhat roundabout
way (you've got two separate assets for setting up this one thing), or
should the USB port code be responsible for explicitly checking if
there are any applicable assets when the port is created?
The advantange of the second approach is that it doesn't involve
checking all the ancestors every time a new device is added (and it
involves only one asset). The disadvantage is that it works only for
USB ports.
To answer the question, we need to know how other subsystems might
want to use the asset-matching approach. My guess is that only a small
number of device types would care about assets (in the same way that
assets matter to USB ports but not to other USB device types). And it
might not be necessary to check against every ancestor; we might know
beforehand where to check (just as the USB port would know to look for
an asset attached to the host controller device).
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices
generic_pm_domain is overkill, so I introduced power controller which only focuses on power on/off and being shared by multiple devices. Even though it is overkill, it may be better than introducing a new abstraction. After all, this is exactly the sort of thing that PM domains were originally created to handle. Rafael, do you have any advice on this? The generic_pm_domain structure is fairly complicated, there's a lot of code in drivers/base/power/domain.c (it's the biggest source file in its directory), and I'm not aware of any documentation. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices
On Mon, 3 Dec 2012, Andy Green wrote: Unless someone NAKs it for sure already (if you're already sure you're going to, please do so to avoid wasting time), I'll issue a try#2 of my code later which demonstrates what I mean. At least I guess it's useful for comparative purposes. Before you go writing a whole lot more code, we should discuss the basics a bit more clearly. There are several unsettled issues here: 1. Should the LAN95xx stuff be associated with the ehci-omap.0's driver or with the hub port? The port would be more flexible, offering the ability to turn the power off and on while the system is running without affecting anything else. But the port code is currently in flux, which could cause this new addition to be delayed. (On the other hand, since the LAN95xx is the only thing connected to the root hub, it could be powered off and on by unbinding the ehci-omap.0 device from its driver and rebinding it.) 2. If we do choose the port, do we want to identify it by matching against a device name string or by matching a sequence of port numbers (in this case, a length-1 sequence)? The port numbers are fixed by the board design, whereas the device name strings might get changed in the future. On the other hand, the port numbers apply only to USB whereas device names can be used by any subsystem. 3. Should the matching mechanism go into the device core or into the USB port code? (This is related to the previous question.) 4. Should this be implemented simply as a regulator (or a pair of regulators)? Or should it be generalized to some sort of PM domain thing? The generic_pm_domain structure defined in include/linux/pm_domain.h seems like overkill, but maybe it's the most appropriate thing to use. Until we decide on the answers to these questions, there's no point writing detailed patches. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [try#1 PATCH 5/7] omap4: panda: add smsc95xx regulator and reset dependent on root hub
On Fri, 30 Nov 2012, Andy Green (林安廸) wrote: I have everything discussed above ready for a try#2, including the descendant matching stuff in separate patches. The code got a lot smaller and better with the _ops struct. The new code can attach the assets to the targeted hub port as discussed (using only -0:1.0/port1 and only checking ehci-omap.0 descendants at device_add() time), but the hub port device never probes, unless I missed the point it's because it actually never binds to a driver, it's a very unusual standalone logical device. That's right; it gets registered but it never binds. It doesn't need a driver because all the port-related activity is done by the hub driver. If that's the case I could work around that by doing the probe() asset stuff at device_add() time if there's no driver name on the device, but actually I am not sure that's where we wanted to end up. Now we got so far as to succeed to associate regulator + clock assets to the logical hub port device, isn't it that we want the assets to be enabled and disabled according to hub port power state? In that case it needs to go in the direction of calling device_asset helpers in the hub.c code that handles enable and disable hub port power. Is this sounding like the right way, or something else? I'm not so sure about this. Maybe we're trying to solve too many different kinds of problem at once. The original device asset scheme is fine if all you want to do is turn on the regulator when ehci-omap binds and turn it off when the driver unbinds. But if we want to go farther, maybe the device asset approach isn't the right way. In particular, we would like the regulator to get turned off and on by the port driver as part of its suspend/resume handling. The most straightforward way to do this is to modify the PM code in port.c. This would be specific to USB ports. I toyed around with the idea of a more general-purpose approach using PM assets that could hook into various PM callbacks, but it seemed to get too complicated. And besides, it only ended up looking like a way to put something into a PM domain without the driver's knowledge. So maybe something like Ming Lei's suggestion really would be best. We could add code specifically for regulators and clocks to port.c. Trying to write something that can work for all types of devices, not just USB ports, may be over-reaching. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Thu, 29 Nov 2012, Ming Lei wrote: If we want to set up the association between usb port and power domain, usb knowledge is required, at least bus info and port topology are needed. One difficulty is the fact that the device(such as usb port) is independent with the 'power domain', for example, the device isn't created(ports of the root hub is created after ehci-omap probe, and port device of non-root hub may depend on powering on the power domain) when defining the regulator things, so could we figure out one general way in theory to describe the associated device with the 'power domain'? Andy has tried the wildcard dev path, and port topology string is introduced in my suggestion, looks both are not general. The device path approach probably could be made to work, but it does have the problem that it depends on device names which may change in the future. However, I can't think of any other general-purpose approach. And most especially, I can't think of any approach that doesn't require extra overhead _every_ time a new device is registered. The port topology string is, of course, specific to USB. I admit the suggestion is partial because we still have not a general abstract on 'power domain' in this problem, and once it is ready, the solution might be in a shape at least for USB. In PC world, ACPI might do sort of something of the 'power domain' I'm not worried about the power domain part. For now, a regulator is all we need. It should be easy enough to expand that later on. Maybe we need to create a new thread on this discussion and make more guys involved(PM/USB/driver core/OMAP/). I will study the problem further, and hope I can post something for starting the discussion later. It would be better to have a more general approach. So far nobody has Yes, I agree. IMO, my suggestion is still in the direction to being general, because a general 'power domain' concept is introduced in it, for example the 'struct power_domain' is associated with 'struct port_power_domain'. It's general, but in the wrong direction. The hard part isn't the power domain aspect; it is setting up the match between the power domain and the dynamically created device. I understand the same 'power domain' concept should be applied to other device or bus too, and the power associated with this device can be switched off sometimes too for saving power consumption. But still looks specific device/subsystem knowledge is required to set up the association. Alan, so could the above be your concern on a more general approach? Or you hope a more general way(such as, do it in driver core or dev PM core) to associate the 'power domain' with one device(for example, usb port in the LAN95xx problem) too? Or other things? Right now we don't have _any_ general (i.e., not bus-specific) way to identify individual devices except for the device name strings. I don't know -- maybe this shouldn't have a general solution at all. Maybe we just need a platform-specific way to associate a regulator or clock with a USB port, something that the port driver would know about explicitly. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [try#1 PATCH 5/7] omap4: panda: add smsc95xx regulator and reset dependent on root hub
On Thu, 29 Nov 2012, Andy Green wrote: However I think what you're saying about binding to hub power is good. The hub ports are not devices, but it would be possible to bind an asset array to them and make the pre- and post- code functions. In the 3.6 kernel, hub ports are not devices. In 3.7 they are -- that is, each hub port has its own struct device. I think it will be possible to address objections around the pathiness by being able to seed the path match with a platform_device pointer (there exists in the board file time a platform_device for ehci-omap.0 ...) and just matching the remainder on a single usb device's name, like *-1.1-1. Can you think of a way to do this without checking for a match every time a new device is registered? For instance, in this case it would be preferable to do this match only for descendants of ehci-omap.0. To match the port device, the string would have to be something like *-0:1.0/port2. In fact, if the match were anchored at the end of the string, we wouldn't need the wildcard at all -- at least, not in this case. The string could simply be -0:1.0/port2. But that's only if the match is restricted to devices below ehci-omap.0. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Wed, 28 Nov 2012, Roger Quadros wrote:
board file:
static struct regulator myreg = {
.name = mydevice-regulator,
};
static struct device_asset mydevice_assets[] = {
{
.name = mydevice-regulator,
.handler = regulator_default_asset_handler,
},
{ }
};
static struct platform_device mydevice = {
...
.dev = {
.assets = mydevice_assets,
},
...
};
From Pandaboard's point of view, is mydevice supposed to be referring to
ehci-omap, LAN95xx or something else?
ehci-omap.0.
Strictly speaking, the regulator doesn't belongs neither to ehci-omap
nor LAN95xx. It belongs to a power domain on the board. And user should
have control to switch it OFF when required without hampering operation
of ehci-omap, so that the other USB ports are still usable.
That is the one disadvantage of the approach we are discussing.
But what API would you use to give the user this control? Neither the
GPIO nor the regulator has a struct device, so you can't use sysfs
directly. And even if you could, it would probably be a bad idea
because the user might turn off power to the LAN95xx while the chip was
being used.
The natural answer is to associate the regulator with the USB port that
the LAN95xx is connected to, so that the new port-power mechanism could
provide the control you want. Then how should that association be set
up?
Lei Ming provided a partial answer, but his suggestion is tied to USB.
It would be better to have a more general approach. So far nobody has
come up with a suggestion that everybody approves of.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Tue, 27 Nov 2012, Ming Lei wrote: IMO, all matches mean the devices are inside the ehci-omap bus, so the direct/simple way is to enable/disable the regulators in the probe() and remove() of ehci-omap controller driver. When this discussion started, Felipe argued against such an approach. He pointed out that the same chip could be used in other platforms, and then the code added to ehci-omap.c would have to be duplicated in several other places. We should have a more generic solution in a more central location, like the device core. For example, each struct platform_device could have a list of resources to be enabled when the device is bound to a driver and disabled when the device is unbound. Such a list could include regulators, clocks, and whatever else people can invent. In this case, when it is created the ehci-omap.0 platform device could get an entry pointing to the LAN95xx's regulator. Then the regulator would automatically be turned on when the platform device is bound to the ehci-omap driver. How does this sound? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Tue, 27 Nov 2012, Andy Green wrote: I don't know if such an approach would be sufficiently general for future requirements, but it would solve the problem at hand. We can get a notification about device creation and do things there. But the cost of that is like the tuple suggestion, they'll only be able to do a fixed thing like operate on regulators. I'm not quite sure what you mean by that. _Any_ function is capable of doing only a fixed thing. Actually the targeted device may have arbitrary associated assets like generic GPIO to turn on a flash for built-in webcam controlled out-of-band. These also can be reached by known names. And the driver may wish to do things with them that are more complex than enable / disable or follow logical lifecycle of the hub or whatever. Let's worry about that when it arises. However when the guidance from Greg is that we can do nothing except complain at hardware designers for some reason I failed to quite identify, I fear we are moving deckchairs on the titanic. Greg's advice was simply not to rely on pathnames in sysfs because they aren't fixed in stone. That leaves plenty of other ways to approach this problem. Basically, what you want is for something related to device A (the regulator or the GPIO) to happen whenever device B (the ehci-omap.0 platform device) is bound to a driver. The most straightforward way to arrange this is for A's driver to have a callback that is invoked whenever B is bound or unbound. The most straightforward way to arrange _that_ is to allow each platform_device to have a list of callbacks. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Wed, 28 Nov 2012, Andy Green wrote: Greg's advice was simply not to rely on pathnames in sysfs because they aren't fixed in stone. That leaves plenty of other ways to approach this problem. It's sage advice, but there is zero code provided in my patches that relies on pathnames in sysfs. In your 1/5 patch, _device_path_generate() concatenates device name strings, starting from a device root and separating elements with '/' characters. Isn't that the same as a sysfs pathname? Basically, what you want is for something related to device A (the regulator or the GPIO) to happen whenever device B (the ehci-omap.0 platform device) is bound to a driver. The most straightforward way to arrange this is for A's driver to have a callback that is invoked whenever B is bound or unbound. The most straightforward way to arrange _that_ is to allow each platform_device to have a list of callbacks. Sorry I didn't really understand this proposal yet. You want A, the regulator, driver to grow a callback function that gets called when the targeted platform_device (B, ehci-omap.0) probe happens. Could you expand what the callback prototype or new members in the struct might look like? It's your tuple thing or we pass it an opaque pointer that is the struct regulator * or suchlike? Well, it won't be exactly the same as the tuple thing because no strings will be involved, but it would be similar. The callback would receive an opaque pointer (presumably to the regulator) and a device pointer (the B device). Throwing out the path stuff and limiting this to platform_device means you cannot bind to dynamically created objects like hub or anything downstream of a hub. So Felipe's identification of the hub as the happening place to do this is out of luck. Greg pointed out that this could be useful for arbitrary devices, not just platform devices, so it could be applied to dynamically created objects. As for what Felipe said... He suggested doing this when the hub driver binds to the controller's root hub. The root hub is created when the controller's driver registers the new USB bus. This happens as part of the driver's probe routine. So what I have been talking about is very similar (in terms of when it happens) to what Felipe wanted. Besides, Felipe wasn't thinking in the most general terms. (In fact, at first he confused the root hub with the LAN95xx's hub.) There's no reason to restrict this sort of thing to USB hubs (or to regulators, for that matter). The driver core is the right place for it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Wed, 28 Nov 2012, Andy Green wrote:
OK. So I try to sketch it out iteractively to try to get in sync:
device.h:
enum asset_event {
AE_PROBED,
AE_REMOVED
};
struct device_asset {
char *name; /* name of regulator, clock, etc */
void *asset; /* regulator, clock, etc */
int (*handler)(struct device *dev_owner, enum asset_event
asset_event,
struct device_asset *asset);
};
Another possibility is to have two handlers: one for pre-probe and the
other for post-remove. Then of course asset_event wouldn't be needed.
Linus tends to prefer this strategy -- separate functions for separate
events. That's why struct dev_pm_ops has so many method pointers.
struct device {
...
struct device_asset *assets;
Or a list instead of a NULL-terminated array. It depends on whether
people will want to add or remove assets dynamically. For now an array
would be fine.
...
};
drivers/base/dd.c | really_probe():
...
struct device_asset *asset;
...
asset = dev-assets;
while (asset asset-name) {
Maybe it would be better to test asset-handler. Some assets might not
need names.
if (asset-handler(dev, AE_PROBED, asset)) {
/* clean up and bail */
}
asset++;
}
/* do probe */
...
drivers/base/dd.c | __device_release_driver: (is this really the best
place to oppose probe()?)
The right place is just after the remove method is called, so yes.
...
struct device_asset *asset;
...
/* call device -remove() */
...
asset = dev-assets;
while (asset asset-name) {
asset-handler(dev, AE_REMOVED, asset);
asset++;
}
It would be slightly safer to iterate in reverse order.
...
board file:
static struct regulator myreg = {
.name = mydevice-regulator,
};
static struct device_asset mydevice_assets[] = {
{
.name = mydevice-regulator,
.handler = regulator_default_asset_handler,
},
{ }
};
static struct platform_device mydevice = {
...
.dev = {
.assets = mydevice_assets,
},
...
};
regulator code:
int regulator_default_asset_handler(struct device *dev_owner, enum
asset_event asset_event, struct device_asset *asset)
{
struct regulator **reg = asset-asset;
int n;
switch (asset_event) {
case AE_PROBED:
*reg = regulator_get(dev_owner, asset-name);
if (IS_ERR(*reg))
return *reg;
PTR_ERR.
n = regulator_enable(*reg);
if (n 0)
regulator_put(*reg);
return n;
case AE_REMOVED:
regulator_put(*reg);
break;
}
return 0;
}
EXPORT_SYMBOL_GPL(regulator_default_asset_handler);
The subsystems that can expect to get used (clock...) might each want to
define a default handler like the one for regulator. That'll be an end
to the code duplication issue. The user can still do his own handler if
he wants.
I put a name field in so we can use regulator_get() nicely, we don't
need access to the object pointer or that it exists at boardfile-time
that way either. But I can see it's arguable.
It hadn't occurred to me, but it seems like a good idea.
Yes, overall this is almost exactly what I had in mind.
Throwing out the path stuff and limiting this to platform_device means
you cannot bind to dynamically created objects like hub or anything
downstream of a hub. So Felipe's identification of the hub as the
happening place to do this is out of luck.
Greg pointed out that this could be useful for arbitrary devices, not
just platform devices, so it could be applied to dynamically created
objects.
Well that is cool, but to exploit that in the dynamic object case
arrangements for identifying the appropriate object has appeared are
needed.
For example, this scheme wouldn't lend itself easily to associating the
regulator with the particular root-hub port that the LAN95xx is
attached to. I can't think of any reasonable way to do that other than
the approaches we have already considered.
We have a nice array of platform_devices nicely there in the
board file we can attach assets to like pdev[1].dev.assets = xxx; but
that's not there in dynamic device case. Anyway this sounds like what
we're discussing can be well worth establishing and might lead to that
later.
Agreed.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http
Re: [RFC PATCH 1/5] drivers : introduce device_path api
On Mon, 26 Nov 2012, Andy Green wrote: This adds a small optional API into drivers/base which deals with generating, matching and registration of wildcard device paths. From a struct device * you can generate a string like /platform/usbhs_omap/ehci-omap.0/usb1/1-1 which enapsulates the path of the device's connection to the board. These can be used to match up other assets, for example struct regulators, that have been registed elsewhere with a device instance that is probed asynchronously from the other board assets. If your device is on a bus, as it probably is, the device path will feature redundant bus indexes that do not contain information about the connectivity. For example if more than one driver can generate devices on the same bus, then the ordering of device probing will change the path, despite the connectivity remains the same. For that reason, to get a deterministic path for matching, wildcards are allowed. If your target device has the path /platform/usbhs_omap/ehci-omap.0/usb1/1-1 generated, in the asset you wish to match with it you can instead use /platform/usbhs_omap/ehci-omap.0/usb*/*-1 in order to only leave the useful, invariant parts of the path to match against. Have you considered limiting these wildcards in various useful ways? In this example, the wildcard must match a string of decimal digits. (Furthermore the two wildcard strings will always match each other, although it's probably not necessary to add this sort of constraint.) I don't know what sort of matches people will want in the future. Perhaps one for hex digits, or one for arbitrary text with the exception of a few characters (such as '/' but perhaps others too). To do what you want for now, the match should be restricted to digits. To avoid having to adapt every kind of search by string api to also use the wildcards, the api takes the approach you can register your wildcard, and if a matching path is generated for a device, the wildcard itself is handed back as the device path instead. So if your board code or a (not yet done) DT binding registers this wildcard /platform/usbhs_omap/ehci-omap.0/usb* and the usb hub driver asks to generate its device path device_path_generate(dev, name, sizeof name); that is actually /platform/usbhs_omap/ehci-omap.0/usb1 then what will be returned is /platform/usbhs_omap/ehci-omap.0/usb* providing the same literal string for ehci-omap.0 hub no matter how many\ usb buses have been registered before. This wildcard string can then be matched to regulators or other string- searchable assets intended for the device on that hardware path. That's not how I would do it, at least, not for this application. I would register tuples containing a name, a pointer, and two callback routines. For example, (/platform/usbhs_omap/ehci-omap.0/usb*, omap_vhub_device, turn_on_regulator, turn_off_regulator) The when a device is registered whose path matches the string in such a tuple, the device core would know to invoke the first callback. The arguments would be a pointer to the device being registered and the pointer in the tuple. Similarly, the device core would invoke the second callback when the device is unregistered. There doesn't have to be anything in this scheme that's specific to hubs or even to USB. In particular, no changes to the hub driver would be needed. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
