+Tero, Tony and some TI folks On 18/06/18 15:21, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@xxxxxx> writes: >> On 18/06/18 12:51, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Johan Hovold <johan@xxxxxxxxxx> writes: >>>> On Mon, Jun 18, 2018 at 12:33:44PM +0300, Felipe Balbi wrote: >>>> >>>>> Johan Hovold <johan@xxxxxxxxxx> writes: >>>> >>>>>> I suggest merging this fix for 4.18-rc, and then Roger can rework the >>>>>> driver so that it works also on OMAP. >>>>> >>>>> omap has its own glue layer for several reasons. If you're talking about >>>>> Keystone devices, then okay, I understand. But in that case, this would >>>>> mean Keystone is copying the same arguably broken PM domain design from >>>>> OMAP and it would be best not to propagate that idea. >>>> >>>> Maybe so. I'm not sure what Roger's use case is, but perhaps the omap >>>> glue driver could be used instead. >>> >>> unlikely. Keystone devices are very different from OMAP family. But >>> we'll see what Roger says. >>> >> >> Well, I was considering to use of-simple for the AM654 SoC [1] but now >> I'm of the opinion that it might be better to add a new glue layer driver > > why isn't dwc3-keystone.c enough? It is doing too many things than we really need to for AM654. > >> for that because >> - it needs to poke a few registers in the wrapper region > > dwc3-keystone.c does that already > >> - it doesn't really need the driver to enable any clock > > Seems to me you're trying to port omap_device to arm64... It isn't an omap_device but is very similar to how it is done on keystone. > >> - it needs a pm_runtime_get_sync() to be done in probe > > this really shouldn't be necessary. Keystone doesn't rely on all the > omap_device legacy. At least it didn't use to. Could it be that you're > just missing a struct dev_pm_domain definition for arm64? I don't think so. If I had missed it it wouldn't enable at all. > > I haven't seen how you guys implemented your PM for arm64 (is there a > publically accessible version somewhere?), but I'd say you should take > the opportunity to remove this relying on pm_runtime_get_sync() calls > from probe and just do what everybody else does; namely: enable clocks > on probe, pm_runtime_set_active, etc. This is something Tero can comment on. > > This helps drivers being able to make assumptions about devices being > enabled during probe. pm_runtime becomes easier to implement generically > too. > You keep mentioning that PM domain design is broken on OMAP. Could you please clarify what is broken? Is it the fact that the bus code doesn't enable the device before probe and that we have to do a pm_runtime_sync() in probe? I tried to discuss this here [1] but looks like you missed it. Re-iterating here. Platform bus doesn't seem to enable the device as part of of_platform_populate(). see __device_attach() and driver_probe_device() in drivers/base/dd.c It does a pm_runtime_get_sync() on dev->parent but not on dev. Also, from section 5 of Documentation/power/runtime_pm.txt "In addition to that, the initial runtime PM status of all devices is 'suspended', but it need not reflect the actual physical state of the device. Thus, if the device is initially active (i.e. it is able to process I/O), its runtime PM status must be changed to 'active', with the help of pm_runtime_set_active(), before pm_runtime_enable() is called for the device." "Note, if the device may execute pm_runtime calls during the probe (such as if it is registers with a subsystem that may call back in) then the pm_runtime_get_sync() call paired with a pm_runtime_put() call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer." So looks like we can't assume that the device is "active" when probe() is called. Which means, we need to do pm_runtime_get_sync(); enable optional clocks; [1] https://lkml.org/lkml/2018/6/13/265 -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
