Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Also, IMHO reset should always be done during probe() so driver can be dead sure that we're starting from a known state. This is even more Depends on the IP block. Eg: you might want to keep the screen showing the contents drawn by the bootloader while booting the kernel and smoothly change to a new image when userspace comes online. In that case you probably don't want to blindly reset the display controller for example. Cheers, Peter. -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Peter == Peter De Schrijver pdeschrij...@nvidia.com writes: Also, IMHO reset should always be done during probe() so driver can be dead sure that we're starting from a known state. This is even more Peter Depends on the IP block. Eg: you might want to keep the screen Peter showing the contents drawn by the bootloader while booting the Peter kernel and smoothly change to a new image when userspace comes Peter online. In that case you probably don't want to blindly reset Peter the display controller for example. Indeed. Another recent example was gpio pins (controlling stuff like reset signals) where the bootloader had already set things up correctly and the reset caused pins to toggle. -- Bye, Peter Korsgaard -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, I think Vaibhav has covered most of the ground here, but just a few comments: On Fri, 15 Feb 2013, Felipe Balbi wrote: On Thu, Feb 14, 2013 at 08:47:53PM +, Paul Walmsley wrote: Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't why not ? It's part of the driver's address space anyway. It's not part of the IP in question (usb, ethernet, etc) but it's part of the TI wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the TI-specific integration registers (revision, sysc, syss...). So they're not part of the licensed IP, but they're part of the TI wrapper around those. Ideally Linux drivers would be as independent as possible from their SoC- or board-specific integration. This includes bus integration, etc. That way, an IP block like UART, where most or all of the registers are shared with the common 8250 code, might be able to use a shared driver. Also, it should be noted that the TI wrapper isn't necessarily common to all TI SoCs :-) The wrapper bits and functions can change from TI SoC to SoC. have anything specifically to do with the underlying device IP block, but of course they do, soft reset touches the underlying IP, so does force idle, no idle, etc. Force idle, no idle, etc. only affects what's reported to the PRCM via the SIdleAck/MStandbyReq lines. It shouldn't affect anything in the IP block itself. As far as reset goes, the chip-wide reset bits affect the IP block too, but we wouldn't put that code into the drivers :-) - Paul -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Wed, Feb 20, 2013 at 05:38:49PM +, Paul Walmsley wrote: Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't just now I noticed this fun fact: driver's should touch *their* SYSCONFIG registers You're stating yourself that SYSCONFIG belongs to the IP and the IP is fully controlled by its driver. ;-) why not ? It's part of the driver's address space anyway. It's not part of the IP in question (usb, ethernet, etc) but it's part of the TI wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the TI-specific integration registers (revision, sysc, syss...). So they're not part of the licensed IP, but they're part of the TI wrapper around those. Ideally Linux drivers would be as independent as possible from their SoC- or board-specific integration. This includes bus integration, etc. That way, an IP block like UART, where most or all of the registers are shared with the common 8250 code, might be able to use a shared driver. right, then why did we even bother adding omap-serial ? If the whole idea was to hide integration, what was the point in introducing omap-serial ? Also, it should be noted that the TI wrapper isn't necessarily common to all TI SoCs :-) The wrapper bits and functions can change from TI SoC to SoC. yeah, that's because of the chassis architecture definition and has little value to current discussion ;-) have anything specifically to do with the underlying device IP block, but of course they do, soft reset touches the underlying IP, so does force idle, no idle, etc. Force idle, no idle, etc. only affects what's reported to the PRCM via the SIdleAck/MStandbyReq lines. It shouldn't affect anything in the IP block itself. It does touch the IP block. After the asynchronous PM handshake (Ack-Req) is successful the IP block isn't in a usable state. As far as reset goes, the chip-wide reset bits affect the IP block too, but we wouldn't put that code into the drivers :-) why not ? What happens when drivers need to reset the IP ? For cases where we need reset in runtime, drivers are the best entities to know exactly where the IP must be reset. Also, IMHO reset should always be done during probe() so driver can be dead sure that we're starting from a known state. This is even more important for the complex IPs which are licensed from RTL vendors and are used in different SoCs. While arch/arm/mach-abc/ resets every IP pior to probe() being called, we can't be sure that arch/arm/mach-xyz/ will do the same thing and imposing such requirement is too difficult. Problem just gets worse when you consider SoCs from completely different architectures (say ARM and Blackfin) using the same IP licensed from a particular RTL vendor. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Wed, 20 Feb 2013, Felipe Balbi wrote: On Wed, Feb 20, 2013 at 05:38:49PM +, Paul Walmsley wrote: Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't just now I noticed this fun fact: driver's should touch *their* SYSCONFIG registers You're stating yourself that SYSCONFIG belongs to the IP and the IP is fully controlled by its driver. ;-) Hehe. I should have used some more convoluted expression :-) Ideally Linux drivers would be as independent as possible from their SoC- or board-specific integration. This includes bus integration, etc. That way, an IP block like UART, where most or all of the registers are shared with the common 8250 code, might be able to use a shared driver. right, then why did we even bother adding omap-serial ? If the whole idea was to hide integration, what was the point in introducing omap-serial ? My distant recollection was that the main impetus at the time was DMA support. Which might be moot, now. Not really my parish. Also, it should be noted that the TI wrapper isn't necessarily common to all TI SoCs :-) The wrapper bits and functions can change from TI SoC to SoC. yeah, that's because of the chassis architecture definition and has little value to current discussion ;-) The point was that we don't want to duplicate that code in each driver for each SoC ... rather, it can live in one place, in some separate integration layer. It does touch the IP block. After the asynchronous PM handshake (Ack-Req) is successful the IP block isn't in a usable state. Usually that's just because the PRCM has cut some clock to the device. You can chat with Benoît about this if you want; we had a discussion last year about it in relation to the 32K_SYNCTIMER. As far as reset goes, the chip-wide reset bits affect the IP block too, but we wouldn't put that code into the drivers :-) why not ? In the OMAP case, the SoC-wide reset bits are in the PRCM somewhere, which in turn assert reset for a wide set of IP blocks. So there's not much point to putting code for that in the drivers. What happens when drivers need to reset the IP ? For cases where we need reset in runtime, drivers are the best entities to know exactly where the IP must be reset. We might be miscommunicating. I was mostly referring to SoC-wide reset in my (somewhat silly) counterfactual. So there are at least three kinds of reset in this context, that I think about: 1. SoC-level reset 2. Bus-level reset 3. Device-level reset Ideally these are all independent, even though they might share some of the same hardware mechanisms. The case that's causing us so much hassle is bus-level reset. The problem is that some of the OMAP devices - say, 7-10% - can't be reset and idled simply with a bus-level reset. I'd consider the exceptions as hardware bugs, but we still have to deal with them. Also, IMHO reset should always be done during probe() so driver can be dead sure that we're starting from a known state. This is even more important for the complex IPs which are licensed from RTL vendors and are used in different SoCs. While arch/arm/mach-abc/ resets every IP pior to probe() being called, we can't be sure that arch/arm/mach-xyz/ will do the same thing and imposing such requirement is too difficult. Problem just gets worse when you consider SoCs from completely different architectures (say ARM and Blackfin) using the same IP licensed from a particular RTL vendor. Or if there's no driver for the device at all :-( - Paul
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Wed, Feb 20, 2013 at 09:16:38PM +0200, Felipe Balbi wrote: Also, IMHO reset should always be done during probe() so driver can be dead sure that we're starting from a known state. This is even more important for the complex IPs which are licensed from RTL vendors and are used in different SoCs. While arch/arm/mach-abc/ resets every IP pior to probe() being called, we can't be sure that arch/arm/mach-xyz/ will do the same thing and imposing such requirement is too difficult. And that's where you're wrong. Virtually nothing in arch/arm/mach-* does any manipulation of hardware resets for individual IP blocks. This just doesn't feature. Mainly because many SoCs generally do not have a way to reset individual IP blocks - normally you find that you get the option of resetting the entire platform including the CPU or nothing at all. And what that means is that we expect to take over hardware in a not-well-defined state and start using it. This is even more complicated by kexec() where we can end up booting a kernel from a previously crashed kernel where the devices are still running from the crashed kernel - and we _hope_ that it works. (It doesn't always - think about devices which DMA into the kernel's memory.) So really... get over the fact that you have reset bits that you can twiddle on your SoC - that's a cool additional feature. Many SoCs that Linux runs on doesn't have that, and you have to start using the hardware from whatever state you find it in. And that means a probe function has to do things like: 1. Mask all interrupts on the device and clean out any pending interrupts. 2. Initialize the settings that it needs on the device to the values it requires. etc. I'm not saying to you to forego the reset - I'm saying that your assertion that everything in Linux starts from a known state is definitely false. -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 12:51]: On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? Yes - for devices with drivers, it will hopefully be called by the driver. I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. In the current design, we might be able to do this during the driver's first call to pm_runtime_get*(). I think that's the first point that we can hook into the PM runtime code. Once the hwmod code is moved out to be a bus, I'm hoping we can do this through the driver making a dev-bus-enable_device() call - similar to the way PCI drivers call pci_enable_device(), but not bus-specific. That should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices to work correctly :-( Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. One thing that's unclear to me for the DT case is how we'll bind the driver-specific parts of the reset code to the device, particularly in the case where there's no driver present. It should be possible to place an initcall in the driver-specific header, but that seems like a hack. Any thoughts on this? 2. We need to be able to reset and idle some hardware even if the driver is not compiled in. Yep. - Paul -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: The other option could be to allow custom ioremap function pointers based on address range in __arm_ioremap_pfn_caller() the same way as the SoC specific static mappings are allowed. That would require adding a function pointer to struct map_desc. Maybe that would do the trick? That sounds fine to me too. To me it makes sense to eventually handle the I/O mappings automatically from data in the DT -- hence the association with a bus device, which should be present in DT data. - Paul -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Tue, Feb 19, 2013 at 03:30:01PM +, Paul Walmsley wrote: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: The other option could be to allow custom ioremap function pointers based on address range in __arm_ioremap_pfn_caller() the same way as the SoC specific static mappings are allowed. That would require adding a function pointer to struct map_desc. Maybe that would do the trick? That sounds fine to me too. To me it makes sense to eventually handle the I/O mappings automatically from data in the DT -- hence the association with a bus device, which should be present in DT data. No. I really don't get why OMAP thinks it's soo special that it needs to go around adding lots and lots of new abstractions all over the place. Indirect ioremap() through a function pointer so you can overload it with OMAP specific crap? No way. Function pointers in map_desc - what the hell for? You must be totally mad if you think that's a way forward, I really don't see how you think this kind of crap would be remotely acceptable. Ok, so you have this problem that you need hwmod to touch a register which is also contained within the device resources as well. That's fine. You can do it one of two ways: 1. Call out from the driver at the appropriate points (which seems to be _after_ you've ioremapped it) to access the register. 2. Find the resource, temporarily map the register, access it, and then unmap it. No requirement what so ever to override ioremap(). AT ALL. So stop this madness now. As for a function pointer in struct map_desc. You're bonkers, the lot of you. map_desc is a structure describing the boot time static mappings which gets discarded. Nothing keeps any references around to it, and it is used at a time when *NOTHING ELSE* should be going on in the kernel other than building those static mappings. Not even any arch code poking about at devices. Or anything like that. Because if you think that's acceptable, then we'll need to flush the cache and TLB after each and every map_desc entry is touched - which brings a whole load of new pain and slowness to the kernel boot. So please, stop this idiotic madness now. If you want such things as pci_enable_device(), then what you're actually asking for is omap_enable_device() for OMAP devices. OMAP devices are already specific enough to OMAP SoCs (god knows, they have really complex and obscure behaviours that no one else in their right mind would want to copy) that calling out to omap specific functions would never really be a problem. No need for any of this crazy dev-bus shit either. -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Russell King - ARM Linux li...@arm.linux.org.uk [130219 07:49]: On Tue, Feb 19, 2013 at 03:30:01PM +, Paul Walmsley wrote: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: The other option could be to allow custom ioremap function pointers based on address range in __arm_ioremap_pfn_caller() the same way as the SoC specific static mappings are allowed. That would require adding a function pointer to struct map_desc. Maybe that would do the trick? That sounds fine to me too. To me it makes sense to eventually handle the I/O mappings automatically from data in the DT -- hence the association with a bus device, which should be present in DT data. No. I really don't get why OMAP thinks it's soo special that it needs to go around adding lots and lots of new abstractions all over the place. Hey it's actually quite the opposite. We're trying to find a generic way of doing things in order to avoid omap specific driver hacks. Indirect ioremap() through a function pointer so you can overload it with OMAP specific crap? No way. Function pointers in map_desc - what the hell for? You must be totally mad if you think that's a way forward, I really don't see how you think this kind of crap would be remotely acceptable. Ok, so you have this problem that you need hwmod to touch a register which is also contained within the device resources as well. That's fine. You can do it one of two ways: 1. Call out from the driver at the appropriate points (which seems to be _after_ you've ioremapped it) to access the register. 2. Find the resource, temporarily map the register, access it, and then unmap it. No requirement what so ever to override ioremap(). AT ALL. So stop this madness now. As for a function pointer in struct map_desc. You're bonkers, the lot of you. map_desc is a structure describing the boot time static mappings which gets discarded. Nothing keeps any references around to it, and it is used at a time when *NOTHING ELSE* should be going on in the kernel other than building those static mappings. Not even any arch code poking about at devices. Or anything like that. Because if you think that's acceptable, then we'll need to flush the cache and TLB after each and every map_desc entry is touched - which brings a whole load of new pain and slowness to the kernel boot. So please, stop this idiotic madness now. OK good point. Also it would be risky for things going wrong trying to try set up more complex stuff that early as then debugging it again depends on DEBUG_LL. If you want such things as pci_enable_device(), then what you're actually asking for is omap_enable_device() for OMAP devices. OMAP devices are already specific enough to OMAP SoCs (god knows, they have really complex and obscure behaviours that no one else in their right mind would want to copy) that calling out to omap specific functions would never really be a problem. I'd rather avoid adding omap_enable_device() calls to drivers as we really want to keep the drivers generic. But maybe there could be some generic bus_enable_device() function pointer that could be populated by the bus code during init. No need for any of this crazy dev-bus shit either. Paul's suggestion on configuring things with pm_runtime_get() seems doable and is generic. Regards, Tony -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Paul Walmsley p...@pwsan.com [130219 07:30]: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 12:51]: On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? Yes - for devices with drivers, it will hopefully be called by the driver. I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. In the current design, we might be able to do this during the driver's first call to pm_runtime_get*(). I think that's the first point that we can hook into the PM runtime code. Sounds doable and generic for now. Once the hwmod code is moved out to be a bus, I'm hoping we can do this through the driver making a dev-bus-enable_device() call - similar to the way PCI drivers call pci_enable_device(), but not bus-specific. That should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices to work correctly :-( Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. One thing that's unclear to me for the DT case is how we'll bind the driver-specific parts of the reset code to the device, particularly in the case where there's no driver present. It should be possible to place an initcall in the driver-specific header, but that seems like a hack. Any thoughts on this? I think we can just initialize the driver-specific reset functions in hwmod code and those can just call the driver specific inline functions as needed in the driver specific header files. 2. We need to be able to reset and idle some hardware even if the driver is not compiled in. Yep. Regards, Tony -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 12:51]: On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? Yes - for devices with drivers, it will hopefully be called by the driver. I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. In the current design, we might be able to do this during the driver's first call to pm_runtime_get*(). I think that's the first point that we can hook into the PM runtime code. Sounds doable and generic for now. this isn't really a nice way IMHO. You will end up with drivers assuming that certain details will be initialized whenever it calls pm_runtime_get() for the first time. Then, for devices such as UART which, when used as console, will not get pm_domain-runtime_resume() called because pm_runtime_set_active() has been called right before. IOW, this will not work for all cases and very soon we will have bug reports. Once the hwmod code is moved out to be a bus, I'm hoping we can do this through the driver making a dev-bus-enable_device() call - similar to the way PCI drivers call pci_enable_device(), but not bus-specific. That should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices to work correctly :-( Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. this wouldn't work. How would you write a reset for i2c which can be called for all instances even when there is no driver loaded ? What would be the arguments to such a call ? If you have something like: static inline void omap_i2c_reset(int bus_nr); you would need to keep track of the bus numbers even when there is no driver around and if there is a driver, you would have to ask i2c-omap.c to track all available instances and convert that number to the proper struct omap_i2c pointer. None of those sound like a good plan IMO. From a driver perspective, we want all our calls to have our private structure or a *dev as argument. Those are only avaiable when driver is around and anything which doesn't take those as argument will force driver to add extra unnecessary churn. One thing that's unclear to me for the DT case is how we'll bind the driver-specific parts of the reset code to the device, particularly in the case where there's no driver present. It should be possible to place an initcall in the driver-specific header, but that seems like a hack. Any thoughts on this? I think we can just initialize the driver-specific reset functions in hwmod code and those can just call the driver specific inline functions as needed in the driver specific header files. please don't. This will not work as you expect, we will just add more special cases to drivers which will be even more complex to maintain. What you want is a generic reset callback somewhere. For cases where there is no driver around, then I'm not sure what to do, but doing early reset sounds like a better plan than late reset, so resetting on omap_device_build_from_dt() sounds better IMHO, although it concerns me cases of -EPROBE_DEFER and what that would mean in the long run... -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Felipe Balbi ba...@ti.com [130219 09:01]: Hi, On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 12:51]: On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? Yes - for devices with drivers, it will hopefully be called by the driver. I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. In the current design, we might be able to do this during the driver's first call to pm_runtime_get*(). I think that's the first point that we can hook into the PM runtime code. Sounds doable and generic for now. this isn't really a nice way IMHO. You will end up with drivers assuming that certain details will be initialized whenever it calls pm_runtime_get() for the first time. Then, for devices such as UART which, when used as console, will not get pm_domain-runtime_resume() called because pm_runtime_set_active() has been called right before. IOW, this will not work for all cases and very soon we will have bug reports. Some generic way to init this from drivers is needed, I'm out of ideas here though. Once the hwmod code is moved out to be a bus, I'm hoping we can do this through the driver making a dev-bus-enable_device() call - similar to the way PCI drivers call pci_enable_device(), but not bus-specific. That should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices to work correctly :-( Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. this wouldn't work. How would you write a reset for i2c which can be called for all instances even when there is no driver loaded ? What would be the arguments to such a call ? If you have something like: static inline void omap_i2c_reset(int bus_nr); you would need to keep track of the bus numbers even when there is no driver around and if there is a driver, you would have to ask i2c-omap.c to track all available instances and convert that number to the proper struct omap_i2c pointer. None of those sound like a good plan IMO. From a driver perspective, we want all our calls to have our private structure or a *dev as argument. Those are only avaiable when driver is around and anything which doesn't take those as argument will force driver to add extra unnecessary churn. If we cannot find a clean way to reset unclaimed devices without duplicating driver specific code at the bus level, then let's just assume the drivers need to probe to reset them even if the device is not used on a particular board. One thing that's unclear to me for the DT case is how we'll bind the driver-specific parts of the reset code to the device, particularly in the case where there's no driver present. It should be possible to place an initcall in the driver-specific header, but that seems like a hack. Any thoughts on this? I think we can just initialize the driver-specific reset functions in hwmod code and those can just call the driver specific inline functions as needed in the driver specific header files. please don't. This will not work as you expect, we will just add more special cases to drivers which will be even more complex to maintain. What you want is a generic reset callback
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Tue, Feb 19, 2013 at 08:30:53AM -0800, Tony Lindgren wrote:
* Russell King - ARM Linux li...@arm.linux.org.uk [130219 07:49]:
If you want such things as pci_enable_device(), then what you're actually
asking for is omap_enable_device() for OMAP devices. OMAP devices are
already specific enough to OMAP SoCs (god knows, they have really complex
and obscure behaviours that no one else in their right mind would want
to copy) that calling out to omap specific functions would never really
be a problem.
I'd rather avoid adding omap_enable_device() calls to drivers as we really
want to keep the drivers generic. But maybe there could be some generic
bus_enable_device() function pointer that could be populated by the bus
code during init.
What you're not getting is that pci_enable_device() is a PCI thing which
is mostly a no-op - and where it isn't a no-op, it's something that must
be done _before_ PCI resources are used or even reserved (because
conceptually, this is to do with getting the resources correct.)
The PCI case is:
pci_device_probe(pci_dev)
{
pci_enable_device(pci_dev);
pci_request_regions(pci_dev);
regs = pci_iomap(pci_dev, BAR, size);
...
}
That's different from what you're wanting on OMAP - what you're wanting
there is some way to record the platform device has been ioremapped, so
that you can then fiddle with its idle/reset register from bus code.
If you think about it in light of the above sequence, the enable device
stage *doesn't* suit your needs because that happens before the driver
has done anything.
However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again. Remember, churn is evil. I've
complained to you about the amount of churn that OMAP manufactures
in the past. Linus has complained about it too. You can't continue
like this.
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Tue, Feb 19, 2013 at 09:43:36AM -0800, Tony Lindgren wrote: On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 12:51]: On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? Yes - for devices with drivers, it will hopefully be called by the driver. I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. In the current design, we might be able to do this during the driver's first call to pm_runtime_get*(). I think that's the first point that we can hook into the PM runtime code. Sounds doable and generic for now. this isn't really a nice way IMHO. You will end up with drivers assuming that certain details will be initialized whenever it calls pm_runtime_get() for the first time. Then, for devices such as UART which, when used as console, will not get pm_domain-runtime_resume() called because pm_runtime_set_active() has been called right before. IOW, this will not work for all cases and very soon we will have bug reports. Some generic way to init this from drivers is needed, I'm out of ideas here though. that's fair, but exposing driver-specific details via a static inline in a header which might or might not be called from hwmod isn't the best choice IMHO. the problems this could cause are numerous, specially when you consider drivers which are shared among multiple architectures (think musb, dwc3, ehci, ohci, xhci, ahci_platform, etc). Once the hwmod code is moved out to be a bus, I'm hoping we can do this through the driver making a dev-bus-enable_device() call - similar to the way PCI drivers call pci_enable_device(), but not bus-specific. That should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices to work correctly :-( Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. this wouldn't work. How would you write a reset for i2c which can be called for all instances even when there is no driver loaded ? What would be the arguments to such a call ? If you have something like: static inline void omap_i2c_reset(int bus_nr); you would need to keep track of the bus numbers even when there is no driver around and if there is a driver, you would have to ask i2c-omap.c to track all available instances and convert that number to the proper struct omap_i2c pointer. None of those sound like a good plan IMO. From a driver perspective, we want all our calls to have our private structure or a *dev as argument. Those are only avaiable when driver is around and anything which doesn't take those as argument will force driver to add extra unnecessary churn. If we cannot find a clean way to reset unclaimed devices without duplicating driver specific code at the bus level, then let's just assume the drivers need to probe to reset them even if the device is not used on a particular board. that wouldn't work either. Whenever you have a product which wants to optimize memory footprint, will start by disabling unused modules, then all of your problems are back and we will start to receive bug reports because of an artificial requirement
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
[...] Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. This thread is going in many directions and I've lost track. I think we need to separate out the specific reset cases and look at examples, since there are not very many (in theory). Specifically, IMO, we need to separate out the init-time reset needs from the runtime reset needs. It seems to me that init-time reset issues can (and should) be handled by omap_device/omap_hwmod. For devices with drivers, this can be done easily before driver probe (using bus notifiers). Any remaining devices that specifically need reset can be reset later (which devices are these?) The other problem is the where reset is need during runtime. Again, what are the specific examples here? The one I can think of off the top of my head is I2C, where it's needed in certain error recovery scenarios. Here, we need a way for the driver itself to initiate a full reset. Maybe a new runtime PM hook like -runtime_reset() as Felipe has suggested (though I'm not yet sure runtime PM is the right place for this, since it's not strictly related to runtime PM). Or, if these are mostly corner-case, error recovery scenarios, maybe a a way force the driver to remove itself and re-probe (which would trigger the bus-notifier based reset) as Tony has suggested is the better approach. Either way, it would be good to summarize the devices that need this to be sure what the actual needs are. The OMAP I2C driver is doing a reset and fully re-initializing itself, so it seems the forced remove/probe is the right approach there. Any one have the other use cases handy? Kevin -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Russell King - ARM Linux li...@arm.linux.org.uk [130219 10:26]:
On Tue, Feb 19, 2013 at 08:30:53AM -0800, Tony Lindgren wrote:
* Russell King - ARM Linux li...@arm.linux.org.uk [130219 07:49]:
If you want such things as pci_enable_device(), then what you're actually
asking for is omap_enable_device() for OMAP devices. OMAP devices are
already specific enough to OMAP SoCs (god knows, they have really complex
and obscure behaviours that no one else in their right mind would want
to copy) that calling out to omap specific functions would never really
be a problem.
I'd rather avoid adding omap_enable_device() calls to drivers as we really
want to keep the drivers generic. But maybe there could be some generic
bus_enable_device() function pointer that could be populated by the bus
code during init.
What you're not getting is that pci_enable_device() is a PCI thing which
is mostly a no-op - and where it isn't a no-op, it's something that must
be done _before_ PCI resources are used or even reserved (because
conceptually, this is to do with getting the resources correct.)
The PCI case is:
pci_device_probe(pci_dev)
{
pci_enable_device(pci_dev);
pci_request_regions(pci_dev);
regs = pci_iomap(pci_dev, BAR, size);
...
}
That's different from what you're wanting on OMAP - what you're wanting
there is some way to record the platform device has been ioremapped, so
that you can then fiddle with its idle/reset register from bus code.
If you think about it in light of the above sequence, the enable device
stage *doesn't* suit your needs because that happens before the driver
has done anything.
Right.. that won't help then. It sounds like the proper place would
be something like pm_runtime_init() as these register manage things
like autoidle etc.
However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again. Remember, churn is evil. I've
complained to you about the amount of churn that OMAP manufactures
in the past. Linus has complained about it too. You can't continue
like this.
I don't think there's any churn needed here if done properly. It's
mostly a question of dropping duplicate data from hwmod that we
already have available in device tree. That means we can shrink the
hwmod data needed.
Regards,
Tony
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Tue, Feb 19, 2013 at 11:16:38AM -0800, Kevin Hilman wrote: [ ... ] The other problem is the where reset is need during runtime. Again, what are the specific examples here? The one I can think of off the top of my head is I2C, where it's needed in certain error recovery scenarios. right, I still have a theory that it's not needed in that case either, though I haven't had time to try that out. Here, we need a way for the driver itself to initiate a full reset. Maybe a new runtime PM hook like -runtime_reset() as Felipe has suggested (though I'm not yet sure runtime PM is the right place for this, since it's not strictly related to runtime PM). Or, if these are right, I agree with you but I couldn't think of a better place. Maybe we need a reset hook in struct device itself (as I suggested on another mail) but I'm not sure Greg would take it, unless we have a damn good reason. mostly corner-case, error recovery scenarios, maybe a a way force the driver to remove itself and re-probe (which would trigger the bus-notifier based reset) as Tony has suggested is the better approach. I don't think so. We certainly don't need to go through all memory allocations again. That will just cause unnecessary memory fragmentation. The OMAP I2C driver is doing a reset and fully re-initializing itself, so it seems the forced remove/probe is the right approach there. Any I must disagree here. Doing a reprobe of I2C every time there's an error condition is overkill. It means freeing struct omap_i2c_dev, freeing its IRQ, freeing the ioremapped area, changing mux configuration (due to pinctrl calls) just to do it all over again. Besides omap_i2c_probe() calls omap_i2c_init() which will do the full IP reset anyway. The difference is that calling omap_i2c_init() directly doesn't force us to free and reallocate all resources all over again. one have the other use cases handy? dwc3 gets reset during probe() too, but that has an IP specific reset which doesn't need SYSCONFIG register for that. -- balbi signature.asc Description: Digital signature
hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)
Hi,
On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again. Remember, churn is evil. I've
complained to you about the amount of churn that OMAP manufactures
in the past. Linus has complained about it too. You can't continue
like this.
I don't think there's any churn needed here if done properly. It's
mostly a question of dropping duplicate data from hwmod that we
already have available in device tree. That means we can shrink the
hwmod data needed.
how about starting by removing register addresses and interrupt numbers
which are passed by devicetree today ? If you want to move to DT-only
now even without all drivers DT-adapted, you can have something like
what Marvel folks did for kirkwood:
static void __init kirkwood_dt_init(void)
{
[ ... ]
if (of_machine_is_compatible(globalscale,dreamplug))
dreamplug_init();
if (of_machine_is_compatible(dlink,dns-kirkwood))
dnskw_init();
if (of_machine_is_compatible(iom,iconnect))
iconnect_init();
[ ... ]
of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
}
those machine-based inits are there just to initialize drivers which
aren't converted to DT.
this would let you remove all board-files except board-generic.c and
remove data which is already passed in via DT. It has the benefit of
showing Linus (or whoever cares) that we are working to remove the
churn while also removing some of the pressure of DT conversion, since
there are still details which need to be sorted out (UART function
pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
nodes, etc etc).
Only on board-files we're talking about over 13K lines:
arch/arm/mach-omap2/board-2430sdp.c | 289 --
arch/arm/mach-omap2/board-3430sdp.c | 602
arch/arm/mach-omap2/board-3630sdp.c | 216 -
arch/arm/mach-omap2/board-4430sdp.c | 730 ---
arch/arm/mach-omap2/board-am3517crane.c | 97 --
arch/arm/mach-omap2/board-am3517evm.c| 398
arch/arm/mach-omap2/board-apollon.c | 342 ---
arch/arm/mach-omap2/board-cm-t35.c | 769
arch/arm/mach-omap2/board-cm-t3517.c | 302 --
arch/arm/mach-omap2/board-devkit8000.c | 648 -
arch/arm/mach-omap2/board-flash.c| 242 -
arch/arm/mach-omap2/board-flash.h| 62 --
arch/arm/mach-omap2/board-h4.c | 347 ---
arch/arm/mach-omap2/board-igep0020.c | 673 --
arch/arm/mach-omap2/board-ldp.c | 440 -
arch/arm/mach-omap2/board-n8x0.c | 762 ---
arch/arm/mach-omap2/board-omap3beagle.c | 549 ---
arch/arm/mach-omap2/board-omap3evm.c | 762 ---
arch/arm/mach-omap2/board-omap3logic.c | 249 -
arch/arm/mach-omap2/board-omap3pandora.c | 623 -
arch/arm/mach-omap2/board-omap3stalker.c | 432 -
arch/arm/mach-omap2/board-omap3touchbook.c | 391
arch/arm/mach-omap2/board-omap4panda.c | 467 --
arch/arm/mach-omap2/board-overo.c| 556 ---
arch/arm/mach-omap2/board-rm680.c| 165
arch/arm/mach-omap2/board-rx51-peripherals.c | 1276 --
arch/arm/mach-omap2/board-rx51-video.c | 89 --
arch/arm/mach-omap2/board-rx51.c | 128 ---
arch/arm/mach-omap2/board-rx51.h | 11 -
arch/arm/mach-omap2/board-ti8168evm.c| 62 --
arch/arm/mach-omap2/board-zoom-debugboard.c | 139 ---
arch/arm/mach-omap2/board-zoom-display.c | 147 ---
arch/arm/mach-omap2/board-zoom-peripherals.c | 304 --
arch/arm/mach-omap2/board-zoom.c | 155
arch/arm/mach-omap2/board-zoom.h | 10 -
35 files changed, 13434 deletions(-)
If we remove all addresses and interrupts, numbers look even better.
--
balbi
signature.asc
Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Felipe Balbi ba...@ti.com writes: Hi, On Tue, Feb 19, 2013 at 11:16:38AM -0800, Kevin Hilman wrote: [ ... ] The other problem is the where reset is need during runtime. Again, what are the specific examples here? The one I can think of off the top of my head is I2C, where it's needed in certain error recovery scenarios. right, I still have a theory that it's not needed in that case either, though I haven't had time to try that out. Great, I hope it's not needed. I've asked several times in past as this driver was converted to runtime PM, but was told it was required for various reasons (which I can't remember anymore.) Here, we need a way for the driver itself to initiate a full reset. Maybe a new runtime PM hook like -runtime_reset() as Felipe has suggested (though I'm not yet sure runtime PM is the right place for this, since it's not strictly related to runtime PM). Or, if these are right, I agree with you but I couldn't think of a better place. Maybe we need a reset hook in struct device itself (as I suggested on another mail) but I'm not sure Greg would take it, unless we have a damn good reason. I'm starting to think a -hardreset() method in struct dev_pm_ops is the place to put this. IMO, it needs to be in dev_pm_ops, so it can be selectively overridden by PM domains. On OMAP, we'd just hook it up to omap_device_shutdown() for all omap_devices, and we'd be done. mostly corner-case, error recovery scenarios, maybe a a way force the driver to remove itself and re-probe (which would trigger the bus-notifier based reset) as Tony has suggested is the better approach. I don't think so. We certainly don't need to go through all memory allocations again. That will just cause unnecessary memory fragmentation. The OMAP I2C driver is doing a reset and fully re-initializing itself, so it seems the forced remove/probe is the right approach there. Any I must disagree here. Doing a reprobe of I2C every time there's an error condition is overkill. It means freeing struct omap_i2c_dev, freeing its IRQ, freeing the ioremapped area, changing mux configuration (due to pinctrl calls) just to do it all over again. Besides omap_i2c_probe() calls omap_i2c_init() which will do the full IP reset anyway. The difference is that calling omap_i2c_init() directly doesn't force us to free and reallocate all resources all over again. I agree, that would be a lot of churn. one have the other use cases handy? dwc3 gets reset during probe() too, but that has an IP specific reset which doesn't need SYSCONFIG register for that. OK, but that's still an init-time reset issue Do you know of any other runtime reset cases? If I2C is the only one, then maybe seeing if it can be removed is the right first step. If that works, we don't need any of this. Kevin -- 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
OMAP reset requirements (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency) :x
Hi, On Tue, Feb 19, 2013 at 11:50:37AM -0800, Kevin Hilman wrote: The other problem is the where reset is need during runtime. Again, what are the specific examples here? The one I can think of off the top of my head is I2C, where it's needed in certain error recovery scenarios. right, I still have a theory that it's not needed in that case either, though I haven't had time to try that out. Great, I hope it's not needed. I've asked several times in past as this driver was converted to runtime PM, but was told it was required for various reasons (which I can't remember anymore.) :-) just like I2C's revision register was broken and had no useful data or MUSB mode1 couldn't be made to work... right. Looking at the functional spec, there's no mention that we need to driver reset signal anywhere, we _do_ need to reinitialize the transfer-related registers (I2C_CON, I2C_CNT, I2C_SA), other than that, I can't think of a need for driving the reset signal. The bus specification isn't that stupid right ? Arbitration Lost and NACK are both expected bus conditions. My only concern is write underflow and read overflow, but I haven't been able to force that to happen yet. Even in those cases, I'd be very surprised if we really needed to driver the reset signal. Here, we need a way for the driver itself to initiate a full reset. Maybe a new runtime PM hook like -runtime_reset() as Felipe has suggested (though I'm not yet sure runtime PM is the right place for this, since it's not strictly related to runtime PM). Or, if these are right, I agree with you but I couldn't think of a better place. Maybe we need a reset hook in struct device itself (as I suggested on another mail) but I'm not sure Greg would take it, unless we have a damn good reason. I'm starting to think a -hardreset() method in struct dev_pm_ops is the place to put this. IMO, it needs to be in dev_pm_ops, so it can be selectively overridden by PM domains. On OMAP, we'd just hook it up to omap_device_shutdown() for all omap_devices, and we'd be done. do you mean omap_device_shutdown(); omap_device_enable(); ? one have the other use cases handy? dwc3 gets reset during probe() too, but that has an IP specific reset which doesn't need SYSCONFIG register for that. OK, but that's still an init-time reset issue yeah, and this is pretty mandatory otherwise dwc3 and its PHYs (USB3 pair plus USB2) won't synchronize and we'll have all sorts of issues :-) Do you know of any other runtime reset cases? If I2C is the only one, then maybe seeing if it can be removed is the right first step. If that works, we don't need any of this. I don't know of any other, but there might be a few... On the other hand, I doubt there's any of them which is truly necessary, unless there's a bug in the IP which we can solve by issuing a reset, other than that, driving the reset signal shouldn't be necessary outside of probe() (or something before probe()). -- balbi signature.asc Description: Digital signature
Re: hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)
* Felipe Balbi ba...@ti.com [130219 11:47]:
Hi,
On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again. Remember, churn is evil. I've
complained to you about the amount of churn that OMAP manufactures
in the past. Linus has complained about it too. You can't continue
like this.
I don't think there's any churn needed here if done properly. It's
mostly a question of dropping duplicate data from hwmod that we
already have available in device tree. That means we can shrink the
hwmod data needed.
how about starting by removing register addresses and interrupt numbers
which are passed by devicetree today ? If you want to move to DT-only
now even without all drivers DT-adapted, you can have something like
what Marvel folks did for kirkwood:
static void __init kirkwood_dt_init(void)
{
[ ... ]
if (of_machine_is_compatible(globalscale,dreamplug))
dreamplug_init();
if (of_machine_is_compatible(dlink,dns-kirkwood))
dnskw_init();
if (of_machine_is_compatible(iom,iconnect))
iconnect_init();
[ ... ]
of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
}
those machine-based inits are there just to initialize drivers which
aren't converted to DT.
Yes we could do that at least partially, but..
this would let you remove all board-files except board-generic.c and
remove data which is already passed in via DT. It has the benefit of
showing Linus (or whoever cares) that we are working to remove the
churn while also removing some of the pressure of DT conversion, since
there are still details which need to be sorted out (UART function
pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
nodes, etc etc).
..that means massive amount of churn in the board-*.c files to convert
them to various init functions to be called from board-generic.c and
removing the ones that are working with DT.
I think we're better off making first sure things are usable with
DT, then just dropping the board-*.c files as we go.
And omap4 is the place to start as we only have blaze and panda
board files. Once DSS, USB and WLAN work with the .dts files, we
can just drop those board files and make omap4 DT only.
We may be able to drop omap4 board-*.c files faster than going full
DT with few selected legacy init functions in board-generic.c for
things like LCD panel configuration etc.
Only on board-files we're talking about over 13K lines:
arch/arm/mach-omap2/board-2430sdp.c | 289 --
arch/arm/mach-omap2/board-3430sdp.c | 602
arch/arm/mach-omap2/board-3630sdp.c | 216 -
arch/arm/mach-omap2/board-4430sdp.c | 730 ---
arch/arm/mach-omap2/board-am3517crane.c | 97 --
arch/arm/mach-omap2/board-am3517evm.c| 398
arch/arm/mach-omap2/board-apollon.c | 342 ---
arch/arm/mach-omap2/board-cm-t35.c | 769
arch/arm/mach-omap2/board-cm-t3517.c | 302 --
arch/arm/mach-omap2/board-devkit8000.c | 648 -
arch/arm/mach-omap2/board-flash.c| 242 -
arch/arm/mach-omap2/board-flash.h| 62 --
arch/arm/mach-omap2/board-h4.c | 347 ---
arch/arm/mach-omap2/board-igep0020.c | 673 --
arch/arm/mach-omap2/board-ldp.c | 440 -
arch/arm/mach-omap2/board-n8x0.c | 762 ---
arch/arm/mach-omap2/board-omap3beagle.c | 549 ---
arch/arm/mach-omap2/board-omap3evm.c | 762 ---
arch/arm/mach-omap2/board-omap3logic.c | 249 -
arch/arm/mach-omap2/board-omap3pandora.c | 623 -
arch/arm/mach-omap2/board-omap3stalker.c | 432 -
arch/arm/mach-omap2/board-omap3touchbook.c | 391
arch/arm/mach-omap2/board-omap4panda.c | 467 --
arch/arm/mach-omap2/board-overo.c| 556 ---
arch/arm/mach-omap2/board-rm680.c| 165
arch/arm/mach-omap2/board-rx51-peripherals.c | 1276
--
arch/arm/mach-omap2/board-rx51-video.c | 89 --
arch/arm/mach-omap2/board-rx51.c | 128 ---
arch/arm/mach-omap2/board-rx51.h | 11 -
arch/arm/mach-omap2/board-ti8168evm.c| 62 --
arch/arm/mach-omap2/board-zoom-debugboard.c | 139 ---
arch/arm/mach-omap2/board-zoom-display.c | 147 ---
arch/arm/mach-omap2/board-zoom-peripherals.c | 304 --
arch/arm/mach-omap2/board-zoom.c | 155
arch/arm/mach-omap2/board-zoom.h | 10 -
35 files changed, 13434 deletions(-)
If we remove all addresses and interrupts, numbers
Re: hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)
Hi,
On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote:
On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again. Remember, churn is evil. I've
complained to you about the amount of churn that OMAP manufactures
in the past. Linus has complained about it too. You can't continue
like this.
I don't think there's any churn needed here if done properly. It's
mostly a question of dropping duplicate data from hwmod that we
already have available in device tree. That means we can shrink the
hwmod data needed.
how about starting by removing register addresses and interrupt numbers
which are passed by devicetree today ? If you want to move to DT-only
now even without all drivers DT-adapted, you can have something like
what Marvel folks did for kirkwood:
static void __init kirkwood_dt_init(void)
{
[ ... ]
if (of_machine_is_compatible(globalscale,dreamplug))
dreamplug_init();
if (of_machine_is_compatible(dlink,dns-kirkwood))
dnskw_init();
if (of_machine_is_compatible(iom,iconnect))
iconnect_init();
[ ... ]
of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
}
those machine-based inits are there just to initialize drivers which
aren't converted to DT.
Yes we could do that at least partially, but..
this would let you remove all board-files except board-generic.c and
remove data which is already passed in via DT. It has the benefit of
showing Linus (or whoever cares) that we are working to remove the
churn while also removing some of the pressure of DT conversion, since
there are still details which need to be sorted out (UART function
pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
nodes, etc etc).
..that means massive amount of churn in the board-*.c files to convert
them to various init functions to be called from board-generic.c and
removing the ones that are working with DT.
why ? I meant that only what's not converted to DT today should be
handled this way. Also, most of the churn is already there
(usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called
from a different place. We don't need to have one function for each
board, however, maybe we could target by-soc:
if (of_is_compatible(omap3))
omap3_init_devices(); /* or whatever you wanna call it */
omap_init_devices() has initialization for everything which isn't DT
adapted today and as we move things to DT, there's a single place to
remove code from.
I think we're better off making first sure things are usable with
DT, then just dropping the board-*.c files as we go.
And omap4 is the place to start as we only have blaze and panda
board files. Once DSS, USB and WLAN work with the .dts files, we
can just drop those board files and make omap4 DT only.
fair enough.
We may be able to drop omap4 board-*.c files faster than going full
DT with few selected legacy init functions in board-generic.c for
things like LCD panel configuration etc.
Only on board-files we're talking about over 13K lines:
arch/arm/mach-omap2/board-2430sdp.c | 289 --
arch/arm/mach-omap2/board-3430sdp.c | 602
arch/arm/mach-omap2/board-3630sdp.c | 216 -
arch/arm/mach-omap2/board-4430sdp.c | 730 ---
arch/arm/mach-omap2/board-am3517crane.c | 97 --
arch/arm/mach-omap2/board-am3517evm.c| 398
arch/arm/mach-omap2/board-apollon.c | 342 ---
arch/arm/mach-omap2/board-cm-t35.c | 769
arch/arm/mach-omap2/board-cm-t3517.c | 302 --
arch/arm/mach-omap2/board-devkit8000.c | 648 -
arch/arm/mach-omap2/board-flash.c| 242 -
arch/arm/mach-omap2/board-flash.h| 62 --
arch/arm/mach-omap2/board-h4.c | 347 ---
arch/arm/mach-omap2/board-igep0020.c | 673 --
arch/arm/mach-omap2/board-ldp.c | 440 -
arch/arm/mach-omap2/board-n8x0.c | 762 ---
arch/arm/mach-omap2/board-omap3beagle.c | 549 ---
arch/arm/mach-omap2/board-omap3evm.c | 762 ---
arch/arm/mach-omap2/board-omap3logic.c | 249 -
arch/arm/mach-omap2/board-omap3pandora.c | 623 -
arch/arm/mach-omap2/board-omap3stalker.c | 432 -
arch/arm/mach-omap2/board-omap3touchbook.c | 391
arch/arm/mach-omap2/board-omap4panda.c | 467 --
arch/arm/mach-omap2/board-overo.c| 556 ---
arch/arm/mach-omap2/board-rm680.c| 165
Re: hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)
* Felipe Balbi ba...@ti.com [130219 14:26]: On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote: ..that means massive amount of churn in the board-*.c files to convert them to various init functions to be called from board-generic.c and removing the ones that are working with DT. why ? I meant that only what's not converted to DT today should be handled this way. Also, most of the churn is already there (usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called from a different place. We don't need to have one function for each board, however, maybe we could target by-soc: if (of_is_compatible(omap3)) omap3_init_devices(); /* or whatever you wanna call it */ omap_init_devices() has initialization for everything which isn't DT adapted today and as we move things to DT, there's a single place to remove code from. And the pdata for that comes from where? :) I think that means converting each board-*.c to device init functions, which leads to the churn I was mentioning.. I think we're better off making first sure things are usable with DT, then just dropping the board-*.c files as we go. And omap4 is the place to start as we only have blaze and panda board files. Once DSS, USB and WLAN work with the .dts files, we can just drop those board files and make omap4 DT only. fair enough. We may be able to drop omap4 board-*.c files faster than going full DT with few selected legacy init functions in board-generic.c for things like LCD panel configuration etc. Only on board-files we're talking about over 13K lines: 35 files changed, 13434 deletions(-) ... If we remove all addresses and interrupts, numbers look even better. Yeah. Let's start with omap4 first when DSS + USB + WLAN work. USB is going to be ready for v3.10, likely Wlan too. That's good news. Maybe we can then have a legacy device pdata init for DSS, and make omap4 DT only for v3.10. Regards, Tony -- 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: hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)
Hi, On Tue, Feb 19, 2013 at 02:31:28PM -0800, Tony Lindgren wrote: * Felipe Balbi ba...@ti.com [130219 14:26]: On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote: ..that means massive amount of churn in the board-*.c files to convert them to various init functions to be called from board-generic.c and removing the ones that are working with DT. why ? I meant that only what's not converted to DT today should be handled this way. Also, most of the churn is already there (usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called from a different place. We don't need to have one function for each board, however, maybe we could target by-soc: if (of_is_compatible(omap3)) omap3_init_devices(); /* or whatever you wanna call it */ omap_init_devices() has initialization for everything which isn't DT adapted today and as we move things to DT, there's a single place to remove code from. And the pdata for that comes from where? :) I think that means converting each board-*.c to device init functions, which leads to the churn I was mentioning.. fair enough, still a lot less churn ;-) -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Wednesday 20 February 2013 12:46 AM, Kevin Hilman wrote: [...] Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. It's been a while since I last tabulated this. But my recollection was that some kind of IP block-specific reset code is needed for about 7% to 10% of the OMAP IP blocks. Yes it's not too many, but the issue there is the driver specific code that's duplicated in both places. And sounds like the solution to that is to make driver specific reset code a static inline function in the driver header as discussed earlier so bus code can call it when there's no driver loaded. This thread is going in many directions and I've lost track. Indeed. I lost track too and almost gave up further reading. Regards, Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Sat, Feb 16, 2013 at 11:18:36, Shilimkar, Santosh wrote: [...] For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Note that if the ioremap matches a static map area there is no cost to ioremap it multiple times. Thats true though now on OMAP we removed most of the static mappings. The main issue is waste of IO space because, we end up mapping same area two times for all the OMAP drivers. This can be optimized with a arch ioremap caller hook but as discussed here, its nice to have rather than something important. Err.. I was looking at the iotable_init for OMAPx in mainline and it looks like most (all?) of the peripherals are already covered in the static mappings? Regards, Vaibhav -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Monday 18 February 2013 01:38 PM, Bedia, Vaibhav wrote: On Sat, Feb 16, 2013 at 11:18:36, Shilimkar, Santosh wrote: [...] For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Note that if the ioremap matches a static map area there is no cost to ioremap it multiple times. Thats true though now on OMAP we removed most of the static mappings. The main issue is waste of IO space because, we end up mapping same area two times for all the OMAP drivers. This can be optimized with a arch ioremap caller hook but as discussed here, its nice to have rather than something important. Mostly L3 and L4 OCP slaves ones are covered because of L3/L4 map. If you boot a full blown kernel, you will see many dynamically allocated as well. Regards, Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Santosh Shilimkar santosh.shilim...@ti.com writes: On Friday 15 February 2013 09:10 PM, Kevin Hilman wrote: Felipe Balbi ba...@ti.com writes: Currently the omap-serial driver will not work properly if booted via DT with CPUIDLE enabled because it depends on function pointers provided by hwmod to change its own SYSCONFIG register. Remove that relyance on hwmod by moving SYSCONFIG handling to driver itself. Note that this also fixes a possible corner case bug where we could be putting UART in Force Idle mode if we called omap_serial_enable_wakeup(up, false) after setting NOIDLE to the idle mode. This is because hwmod has no protection against that situation. NYET-Signed-off-by: Felipe Balbi ba...@ti.com Here's another approach to getting rid of the sysconfig twiddling in the driver. I wrote this some time ago at my former company ;) but don't think I ever got around to posting it. It doesn't solve the whole problem (e.g. doesn't address the context_loss or enable_wakeup func pointers), but at least gets rid of the need for any SYSCONFIG access in the driver for the idle modes. Needs more thorough testing. I posted similar patch series[1] yesterday after testing it on OMAP4/5 devices. OMAP3 testing seems to be ok as well. AM3XXX and OMAP2 test results is what am waiting for. Yeah, I saw yours after I posted mine. Just continue with yours, I'll be glad to have someone else take this on. Good to know that you had similar idea in mind to get rid of UART sysc hackery. Likewise. Kevin -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Felipe Balbi ba...@ti.com writes: Hi, On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote: The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). what's the point in moving it to late_initcall() ? It makes no difference, if no driver binds to that device it will stay in reset anyway. Maybe what we're missing is properly idling (not exactly) all devices before driver probe kicks in. The only problem is that now omap_device_build_from_dt() is called after we notify that a new device/driver pair has been registered with the platform_bus, right ? So we would still need a way to call _reset() for those which are on DTS but don't have a driver bound to them... The only special requirement for reset remains(which today handled by hwmod function calls) is for devices which needs specific reset sequence. And this one can be handled through a runtime_reset() kind of hook. Does that sound reasonable ? Depends on Rafael. If he says no to the -runtime_reset() I suggested earlier, another aproach needs to be found. In any case, -runtime_reset() is only for devices which actually have a driver, so it matters little. Has somone tried this to see if it works on devices with the custom reset hooks? It should be relatively easy to add a -runtime_reset hook, and hook it up to omap_device_shutdown() at the PM domain level. If we have a patch, showing how we use it in some drivers and making the case for why it is needed in some drivers, it will be easier to take this to Rafael and make a case for it. The difficult part is handling special reset requirements for devices without drivers as there'd be no -runtime_reset() to call. IMO, this should not be difficult, as omap_device_shutdown() - omap_hwmod_shutdown() does the heavy lifting here. Also, We already track whether omap_devices have drivers bound, so adding support to reset/shutdown unbound drivers in late init will be straight forward. Kevin -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote: The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). what's the point in moving it to late_initcall() ? It makes no difference, if no driver binds to that device it will stay in reset anyway. Maybe what we're missing is properly idling (not exactly) all devices before driver probe kicks in. The only problem is that now omap_device_build_from_dt() is called after we notify that a new device/driver pair has been registered with the platform_bus, right ? So we would still need a way to call _reset() for those which are on DTS but don't have a driver bound to them... The only special requirement for reset remains(which today handled by hwmod function calls) is for devices which needs specific reset sequence. And this one can be handled through a runtime_reset() kind of hook. Does that sound reasonable ? Depends on Rafael. If he says no to the -runtime_reset() I suggested earlier, another aproach needs to be found. In any case, -runtime_reset() is only for devices which actually have a driver, so it matters little. The difficult part is handling special reset requirements for devices without drivers as there'd be no -runtime_reset() to call. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
+ Rafael, On Saturday 16 February 2013 02:25 PM, Felipe Balbi wrote: Hi, On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote: The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). what's the point in moving it to late_initcall() ? It makes no difference, if no driver binds to that device it will stay in reset anyway. Maybe what we're missing is properly idling (not exactly) all devices before driver probe kicks in. I think it is largely reducing the early init dependencies and also reducing the role of platform code who today takes care of every device idle and reset initialization. That way late_init() will only have to care about the devices which are not probed by drivers. Tony, Is that right ? The only problem is that now omap_device_build_from_dt() is called after we notify that a new device/driver pair has been registered with the platform_bus, right ? So we would still need a way to call _reset() for those which are on DTS but don't have a driver bound to them... The only special requirement for reset remains(which today handled by hwmod function calls) is for devices which needs specific reset sequence. And this one can be handled through a runtime_reset() kind of hook. Does that sound reasonable ? Depends on Rafael. If he says no to the -runtime_reset() I suggested earlier, another aproach needs to be found. In any case, -runtime_reset() is only for devices which actually have a driver, so it matters little. I agree. Looping Rafael to hear from him. To add a bit of context, On OMAP we do have few IP blocks which needs a special reset sequence during init as well as working state of the driver. Some of this was worked around such cases by populating function pointers from platform data which drivers can use. This obviously doesn't scale and won't work with DT based builds. We have been thinking of having a runtime_reset() generic callback which drivers can use. Whats you take on it ? The difficult part is handling special reset requirements for devices without drivers as there'd be no -runtime_reset() to call. I don't think that requirement exists so if we address the driver requirement, we are good. Even otherwise also, it can be managed from platform code. Regards, Sasntosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Sat, Feb 16, 2013 at 02:47:45PM +0530, Santosh Shilimkar wrote: On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote: The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). what's the point in moving it to late_initcall() ? It makes no difference, if no driver binds to that device it will stay in reset anyway. Maybe what we're missing is properly idling (not exactly) all devices before driver probe kicks in. I think it is largely reducing the early init dependencies and also reducing the role of platform code who today takes care of every device idle and reset initialization. That way late_init() will only have to care about the devices which are not probed by drivers. Tony, Is that right ? Makes not much difference, except that you will have to keep track of which devices have gotten a driver probed and which haven't. IMO, it sounds a lot better to reset everything early on, so we know we're starting at a known stage (and thus drop all bootloader dependencies) then to follow the other route. The difficult part is handling special reset requirements for devices without drivers as there'd be no -runtime_reset() to call. I don't think that requirement exists so if we address the driver requirement, we are good. Even otherwise also, it can be managed Look back at what you want to do at late_initcall() time. You want to reset all devices which haven't gotten a driver bound to them. from platform code. right, the you will need even more data in hwmod to let it know about the special devices. /me wonders when the amount of data will actually decrease. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Saturday 16 February 2013 02:52 PM, Felipe Balbi wrote: Hi, On Sat, Feb 16, 2013 at 02:47:45PM +0530, Santosh Shilimkar wrote: On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote: The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). what's the point in moving it to late_initcall() ? It makes no difference, if no driver binds to that device it will stay in reset anyway. Maybe what we're missing is properly idling (not exactly) all devices before driver probe kicks in. I think it is largely reducing the early init dependencies and also reducing the role of platform code who today takes care of every device idle and reset initialization. That way late_init() will only have to care about the devices which are not probed by drivers. Tony, Is that right ? Makes not much difference, except that you will have to keep track of which devices have gotten a driver probed and which haven't. IMO, it sounds a lot better to reset everything early on, so we know we're starting at a known stage (and thus drop all bootloader dependencies) then to follow the other route. I tend to agree with you. This was exactly the reason Paul and Benoit added that support first up as part of early init code. The difficult part is handling special reset requirements for devices without drivers as there'd be no -runtime_reset() to call. I don't think that requirement exists so if we address the driver requirement, we are good. Even otherwise also, it can be managed Look back at what you want to do at late_initcall() time. You want to reset all devices which haven't gotten a driver bound to them. from platform code. right, the you will need even more data in hwmod to let it know about the special devices. /me wonders when the amount of data will actually decrease. Well that is already supported. There is no need to add any additional information. Device which are initialized, there state is set as initialized. So the late_init() will just have to iterate over un-initialised devices. Just to be clear, I am also in favor of just keeping that part as it is today but we were exploring other options based on comments from Tony during OMAP5 data review. Regards, Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Thu, Feb 14, 2013 at 08:47:53PM +, Paul Walmsley wrote: Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as long as the implementation of ioremap() can be overridden by the device's bus. PCI device drivers already do this -- albeit in a PCI-specific way -- with pci_ioremap_bar(): Err no, that's just a helper to assist checking the resource type and remove all those pesky resource size mistakes - see the commit text: PCI: introduce an pci_ioremap(pdev, barnr) function A common thing in many PCI drivers is to ioremap() an entire bar. This is a slightly fragile thing right now, needing both an address and a size, and many driver writers do.. various things there. This patch introduces an pci_ioremap() function taking just a PCI device struct and the bar number as arguments, and figures this all out itself, in one place. In addition, we can add various sanity checks to this function (the patch already checks to make sure that the bar in question really is a MEM bar; few to no drivers do that sort of thing). Hopefully with this type of API we get less chance of mistakes in drivers with ioremap() operations. Signed-off-by: Arjan van de Ven ar...@linux.intel.com Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); No. ioremap() is already generic. We don't need additional layers of indirection per bus type. -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Thu, Feb 14, 2013 at 09:40:29PM +, Paul Walmsley wrote: Hi, On Thu, 14 Feb 2013, Paul Walmsley wrote: So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Something like this is what I was thinking. Obviously there would be more patches needed, for the various arches, etc.; and I'm not convinced that the function pointer init is done at the right time yet. Comments welcome. Not only does this break the ioremap caller stuff, allowing the caller of ioremap() to be recorded, I believe this to be a total abonimation. This might also be useful as a generic replacement for pci_ioremap_bar(). No it isn't. Read the comments about why pci_ioremap_bar() was introduced in its original commit. It's there to reduce the number of common errors with mapping a PCI bar, so it takes the PCI device and the BAR index. It has a totally different interface to standard ioremap(). It can't be twisted into the standard ioremap() interface in any shape or form. diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index ce328c7..303dba0 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -17,6 +17,7 @@ #include linux/fs.h #include linux/vmalloc.h #include linux/sizes.h +#include linux/platform_device.h #include asm/cp15.h #include asm/cputype.h @@ -28,6 +29,7 @@ #include asm/highmem.h #include asm/system_info.h #include asm/traps.h +#include asm/io.h How many more bloody times do I have to tell people about using asm/io.h directly? You've been around for _long_ enough that you well know this by now. There is no excuse for this kind of thing other than shere laziness or down-right sloppyness on your part. If I make a big enough thing about it, hopefully people will become scared to post their un-self-reviewed patches and instead start taking a little more care in future. -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Russell, On Friday 15 February 2013 03:46 PM, Russell King - ARM Linux wrote: On Thu, Feb 14, 2013 at 08:47:53PM +, Paul Walmsley wrote: [..] So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); No. ioremap() is already generic. We don't need additional layers of indirection per bus type. Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Regards, Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion regards Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Felipe Balbi ba...@ti.com writes:
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
NYET-Signed-off-by: Felipe Balbi ba...@ti.com
Here's another approach to getting rid of the sysconfig twiddling in the
driver. I wrote this some time ago at my former company ;) but don't
think I ever got around to posting it.
It doesn't solve the whole problem (e.g. doesn't address the
context_loss or enable_wakeup func pointers), but at least gets rid of
the need for any SYSCONFIG access in the driver for the idle modes.
Needs more thorough testing.
Kevin
From 3d3956472d2375b5ed939d1d0165e439aa47262f Mon Sep 17 00:00:00 2001
From: Kevin Hilman khil...@ti.com
Date: Wed, 26 Sep 2012 18:36:43 -0700
Subject: [PATCH] ARM: OMAP2+: serial/hwmod: use SW supported slave idle for
all UARTs
Due to various errata, we have several hacks involving modifying the
slave idle mode of the UARTs at runtime. Since the UART driver has
been using the autosuspend feature of runtime PM, each UART is kept in
NO_IDLE during activity, and SMART_IDLE during activity.
The omap_hwmod layer has the per-device HWMOD_SWSUP_SIDLE flag which
will have the exact same behavior, with the benefit of simplifying the
device and driver layers. Switch to using hwmod to automatically
manage the slave idle mode.
This also enables the removal of a couple pdata function pointers from
the driver, which facilitates the conversion to device tree.
Signed-off-by: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c | 3 ++
arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 4 +++
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 4 +++
arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 4 +++
arch/arm/mach-omap2/serial.c | 35 --
arch/arm/plat-omap/include/plat/omap-serial.h | 2 --
drivers/tty/serial/omap-serial.c | 23 --
7 files changed, 15 insertions(+), 60 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
index bd9220e..0d3c1a6 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
@@ -509,6 +509,7 @@ struct omap_hwmod omap2xxx_uart1_hwmod = {
},
},
.class = omap2_uart_class,
+ .flags = HWMOD_SWSUP_SIDLE,
};
/* UART2 */
@@ -528,6 +529,7 @@ struct omap_hwmod omap2xxx_uart2_hwmod = {
},
},
.class = omap2_uart_class,
+ .flags = HWMOD_SWSUP_SIDLE,
};
/* UART3 */
@@ -547,6 +549,7 @@ struct omap_hwmod omap2xxx_uart3_hwmod = {
},
},
.class = omap2_uart_class,
+ .flags = HWMOD_SWSUP_SIDLE,
};
/* dss */
diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index 59d5c1c..1a41f93 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -1903,6 +1903,7 @@ static struct omap_hwmod am33xx_uart1_hwmod = {
.modulemode = MODULEMODE_SWCTRL,
},
},
+ .flags = HWMOD_SWSUP_SIDLE,
};
static struct omap_hwmod_irq_info am33xx_uart2_irqs[] = {
@@ -1923,6 +1924,7 @@ static struct omap_hwmod am33xx_uart2_hwmod = {
.modulemode = MODULEMODE_SWCTRL,
},
},
+ .flags = HWMOD_SWSUP_SIDLE,
};
/* uart3 */
@@ -1950,6 +1952,7 @@ static struct omap_hwmod am33xx_uart3_hwmod = {
.modulemode = MODULEMODE_SWCTRL,
},
},
+ .flags = HWMOD_SWSUP_SIDLE,
};
static struct omap_hwmod_irq_info am33xx_uart4_irqs[] = {
@@ -1970,6 +1973,7 @@ static struct omap_hwmod am33xx_uart4_hwmod = {
.modulemode = MODULEMODE_SWCTRL,
},
},
+ .flags = HWMOD_SWSUP_SIDLE,
};
static struct omap_hwmod_irq_info am33xx_uart5_irqs[] = {
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index f67b7ee..99ada6a 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -497,6 +497,7 @@ static struct omap_hwmod omap3xxx_uart1_hwmod = {
},
},
.class = omap2_uart_class,
+ .flags =
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Fri, Feb 15, 2013 at 07:40:04AM -0800, Kevin Hilman wrote: Felipe Balbi ba...@ti.com writes: Currently the omap-serial driver will not work properly if booted via DT with CPUIDLE enabled because it depends on function pointers provided by hwmod to change its own SYSCONFIG register. Remove that relyance on hwmod by moving SYSCONFIG handling to driver itself. Note that this also fixes a possible corner case bug where we could be putting UART in Force Idle mode if we called omap_serial_enable_wakeup(up, false) after setting NOIDLE to the idle mode. This is because hwmod has no protection against that situation. NYET-Signed-off-by: Felipe Balbi ba...@ti.com Here's another approach to getting rid of the sysconfig twiddling in the driver. I wrote this some time ago at my former company ;) but don't think I ever got around to posting it. It doesn't solve the whole problem (e.g. doesn't address the context_loss or enable_wakeup func pointers), but at least gets rid of the need for any SYSCONFIG access in the driver for the idle modes. wakeup is still done by setting smart_idle_wakeup right, at least to some IPs. Following omap_serial_enable_wakeup() calls, I can see that it turns out to fiddle with sysconfig ;-) @@ -229,8 +209,6 @@ static void serial_omap_stop_tx(struct uart_port *port) serial_out(up, UART_IER, up-ier); } - serial_omap_set_forceidle(up); - pm_runtime_mark_last_busy(up-dev); pm_runtime_put_autosuspend(up-dev); } @@ -298,7 +276,6 @@ static void serial_omap_start_tx(struct uart_port *port) pm_runtime_get_sync(up-dev); serial_omap_enable_ier_thri(up); - serial_omap_set_noidle(up); this is no different then the patches Santosh has sent just a few minutes ago, what gives ? It's exactly the same patch... Anyway, I have exposed a problem with this aproach. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Regards, Tony -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Fri, Feb 15, 2013 at 08:30:32AM -0800, Tony Lindgren wrote: * Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). The only problem is that now omap_device_build_from_dt() is called after we notify that a new device/driver pair has been registered with the platform_bus, right ? So we would still need a way to call _reset() for those which are on DTS but don't have a driver bound to them... -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Friday 15 February 2013 09:10 PM, Kevin Hilman wrote: Felipe Balbi ba...@ti.com writes: Currently the omap-serial driver will not work properly if booted via DT with CPUIDLE enabled because it depends on function pointers provided by hwmod to change its own SYSCONFIG register. Remove that relyance on hwmod by moving SYSCONFIG handling to driver itself. Note that this also fixes a possible corner case bug where we could be putting UART in Force Idle mode if we called omap_serial_enable_wakeup(up, false) after setting NOIDLE to the idle mode. This is because hwmod has no protection against that situation. NYET-Signed-off-by: Felipe Balbi ba...@ti.com Here's another approach to getting rid of the sysconfig twiddling in the driver. I wrote this some time ago at my former company ;) but don't think I ever got around to posting it. It doesn't solve the whole problem (e.g. doesn't address the context_loss or enable_wakeup func pointers), but at least gets rid of the need for any SYSCONFIG access in the driver for the idle modes. Needs more thorough testing. I posted similar patch series[1] yesterday after testing it on OMAP4/5 devices. OMAP3 testing seems to be ok as well. AM3XXX and OMAP2 test results is what am waiting for. Good to know that you had similar idea in mind to get rid of UART sysc hackery. Regards, Santosh [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85177.html -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Friday 15 February 2013 10:00 PM, Tony Lindgren wrote: * Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Right. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. Driver idle functions is getting sorted out. There is no need for driver to fiddle around it since PM runtime back-end can take care of of it. One attempt to clean that is here [1] For the reset, we need to have a generic runtime hook as discussed and that should take care of reset as well. That way driver just calls generic For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Good to know. So overall, I think we do have a way to move forward and get right things done. Regards, Santosh [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg85177.html -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Fri, 15 Feb 2013, Tony Lindgren wrote: * Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Note that if the ioremap matches a static map area there is no cost to ioremap it multiple times. Nicolas -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Saturday 16 February 2013 11:06 AM, Nicolas Pitre wrote: On Fri, 15 Feb 2013, Tony Lindgren wrote: * Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. For the duplicate ioremapping, I don't think there's any need to do it if we get things right. Note that if the ioremap matches a static map area there is no cost to ioremap it multiple times. Thats true though now on OMAP we removed most of the static mappings. The main issue is waste of IO space because, we end up mapping same area two times for all the OMAP drivers. This can be optimized with a arch ioremap caller hook but as discussed here, its nice to have rather than something important. Regards, Santosh -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Friday 15 February 2013 10:12 PM, Felipe Balbi wrote: Hi, On Fri, Feb 15, 2013 at 08:30:32AM -0800, Tony Lindgren wrote: * Santosh Shilimkar santosh.shilim...@ti.com [130215 05:34]: On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote: On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote: Whats your view on use of arch_ioremap_caller() hook ? This can allow us to avoid the dual ioremap() issue discussed here if the hook maintains the list of mapped ios. I was even thinking of having such intelligence within the core ioremap code but thought that might be too invasive. Why do you even need it? There's no problem with ioremapping the same space multiple times (you end up with multiple mappings but that shouldn't be a problem, except for the additional space used.) It just waste of iospace and Tony insisted to have just single ioremap() hence all this discussion The main goal is to avoid duplicating data both in hwmod and DT. That's pretty much solved as we can have the driver probe populate the common data for hwmod from DT as Santosh has already demonstrated. Then we also want the driver specific idle and reset code to be done in the drivers rather than in hwmod and glue it together with hwmod using runtime PM. The biggest issue there is how do we reset and idle some piece of hardware for PM purposes when there's no driver loaded. right, this will be a tough nut to crack. I guess the only way would be reset all IPs early in the boot process, before even creating the platform-devices. Can we do that ? I mean, from omap_device_build_from_dt() we have access to address space of all devices, through ti,hwmods we can figure out which hwmods are linked to that particular device, so whenever you build a device, you could just call _reset(). Thats what we do today and it works perfectly. As per Tony's suggestion, we need to move the non-probed devices reset and idle setup to late_init which is also doable. In that case when probed driver calls runtime_get(), we reset that device and setup the idle settings. And remainder of the devices are managed in late_init(). The only problem is that now omap_device_build_from_dt() is called after we notify that a new device/driver pair has been registered with the platform_bus, right ? So we would still need a way to call _reset() for those which are on DTS but don't have a driver bound to them... The only special requirement for reset remains(which today handled by hwmod function calls) is for devices which needs specific reset sequence. And this one can be handled through a runtime_reset() kind of hook. Does that sound reasonable ? Regards, Santosh -- 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
[RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
NYET-Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/tty/serial/omap-serial.c | 67 +++-
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 57d6b29..078beb1 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -96,6 +96,18 @@
#define OMAP_UART_TCR_TRIG 0x0F
+#define OMAP_UART_SYSC 0x54
+#define OMAP_UART_AUTOIDLE (1 0)
+#define OMAP_UART_SOFTRESET(1 1)
+#define OMAP_UART_ENAWAKEUP(1 2)
+
+#define OMAP_UART_IDLEMODE(n) (((n) 3) 3)
+#define OMAP_UART_FORCE_IDLE OMAP_UART_IDLEMODE(0)
+#define OMAP_UART_NO_IDLE OMAP_UART_IDLEMODE(1)
+#define OMAP_UART_SMART_IDLE OMAP_UART_IDLEMODE(2)
+#define OMAP_UART_SMART_IDLE_WKUP OMAP_UART_IDLEMODE(3)
+#define OMAP_UART_IDLEMODE_MASKOMAP_UART_SMART_IDLE_WKUP
+
struct uart_omap_dma {
u8 uart_dma_tx;
u8 uart_dma_rx;
@@ -191,44 +203,39 @@ static inline void serial_omap_clear_fifos(struct
uart_omap_port *up)
serial_out(up, UART_FCR, 0);
}
-static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
-{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
-
- if (!pdata || !pdata-get_context_loss_count)
- return 0;
-
- return pdata-get_context_loss_count(up-dev);
-}
-
static void serial_omap_set_forceidle(struct uart_omap_port *up)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
-
- if (!pdata || !pdata-set_forceidle)
- return;
+ u32 reg;
- pdata-set_forceidle(up-dev);
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+ reg |= OMAP_UART_FORCE_IDLE;
+ serial_out(up, OMAP_UART_SYSC, reg);
}
static void serial_omap_set_noidle(struct uart_omap_port *up)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
+ u32 reg;
- if (!pdata || !pdata-set_noidle)
- return;
-
- pdata-set_noidle(up-dev);
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+ reg |= OMAP_UART_NO_IDLE;
+ serial_out(up, OMAP_UART_SYSC, reg);
}
static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
+ u32 reg;
- if (!pdata || !pdata-enable_wakeup)
- return;
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+
+ if (enable)
+ reg |= OMAP_UART_SMART_IDLE_WKUP;
+ else
+ reg |= OMAP_UART_SMART_IDLE;
- pdata-enable_wakeup(up-dev, enable);
+ serial_out(up, OMAP_UART_SYSC, reg);
}
/*
@@ -1596,8 +1603,6 @@ static int serial_omap_runtime_suspend(struct device *dev)
if (!pdata)
return 0;
- up-context_loss_cnt = serial_omap_get_context_loss_count(up);
-
if (device_may_wakeup(dev)) {
if (!up-wakeups_enabled) {
serial_omap_enable_wakeup(up, true);
@@ -1620,15 +1625,7 @@ static int serial_omap_runtime_resume(struct device *dev)
{
struct uart_omap_port *up = dev_get_drvdata(dev);
- int loss_cnt = serial_omap_get_context_loss_count(up);
-
- if (loss_cnt 0) {
- dev_err(dev, serial_omap_get_context_loss_count failed : %d\n,
- loss_cnt);
- serial_omap_restore_context(up);
- } else if (up-context_loss_cnt != loss_cnt) {
- serial_omap_restore_context(up);
- }
+ serial_omap_restore_context(up);
up-latency = up-calc_latency;
schedule_work(up-qos_work);
--
1.8.1.rc1.5.g7e0651a
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Felipe Balbi ba...@ti.com [130214 03:19]:
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
I agree the sysconfig stuff should be handled in the drivers.
But considering that we need to also reset or idle hardware
that may not have a driver loaded, it's best to put the
reset/idle function in to the driver specific header as
an inline function.
This way the hwmod code can idle or reset the unclaimed
hardware in a late_initcall.
And probably the handling of sysconfig bits should be
done using a common header in include/linux/omap-hwmod.h
or something so it can be shared between drivers.
Regards,
Tony
NYET-Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/tty/serial/omap-serial.c | 67
+++-
1 file changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c
b/drivers/tty/serial/omap-serial.c
index 57d6b29..078beb1 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -96,6 +96,18 @@
#define OMAP_UART_TCR_TRIG 0x0F
+#define OMAP_UART_SYSC 0x54
+#define OMAP_UART_AUTOIDLE (1 0)
+#define OMAP_UART_SOFTRESET (1 1)
+#define OMAP_UART_ENAWAKEUP (1 2)
+
+#define OMAP_UART_IDLEMODE(n)(((n) 3) 3)
+#define OMAP_UART_FORCE_IDLE OMAP_UART_IDLEMODE(0)
+#define OMAP_UART_NO_IDLEOMAP_UART_IDLEMODE(1)
+#define OMAP_UART_SMART_IDLE OMAP_UART_IDLEMODE(2)
+#define OMAP_UART_SMART_IDLE_WKUP OMAP_UART_IDLEMODE(3)
+#define OMAP_UART_IDLEMODE_MASK OMAP_UART_SMART_IDLE_WKUP
+
struct uart_omap_dma {
u8 uart_dma_tx;
u8 uart_dma_rx;
@@ -191,44 +203,39 @@ static inline void serial_omap_clear_fifos(struct
uart_omap_port *up)
serial_out(up, UART_FCR, 0);
}
-static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
-{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
-
- if (!pdata || !pdata-get_context_loss_count)
- return 0;
-
- return pdata-get_context_loss_count(up-dev);
-}
-
static void serial_omap_set_forceidle(struct uart_omap_port *up)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
-
- if (!pdata || !pdata-set_forceidle)
- return;
+ u32 reg;
- pdata-set_forceidle(up-dev);
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+ reg |= OMAP_UART_FORCE_IDLE;
+ serial_out(up, OMAP_UART_SYSC, reg);
}
static void serial_omap_set_noidle(struct uart_omap_port *up)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
+ u32 reg;
- if (!pdata || !pdata-set_noidle)
- return;
-
- pdata-set_noidle(up-dev);
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+ reg |= OMAP_UART_NO_IDLE;
+ serial_out(up, OMAP_UART_SYSC, reg);
}
static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
{
- struct omap_uart_port_info *pdata = up-dev-platform_data;
+ u32 reg;
- if (!pdata || !pdata-enable_wakeup)
- return;
+ reg = serial_in(up, OMAP_UART_SYSC);
+ reg = ~OMAP_UART_IDLEMODE_MASK;
+
+ if (enable)
+ reg |= OMAP_UART_SMART_IDLE_WKUP;
+ else
+ reg |= OMAP_UART_SMART_IDLE;
- pdata-enable_wakeup(up-dev, enable);
+ serial_out(up, OMAP_UART_SYSC, reg);
}
/*
@@ -1596,8 +1603,6 @@ static int serial_omap_runtime_suspend(struct device
*dev)
if (!pdata)
return 0;
- up-context_loss_cnt = serial_omap_get_context_loss_count(up);
-
if (device_may_wakeup(dev)) {
if (!up-wakeups_enabled) {
serial_omap_enable_wakeup(up, true);
@@ -1620,15 +1625,7 @@ static int serial_omap_runtime_resume(struct device
*dev)
{
struct uart_omap_port *up = dev_get_drvdata(dev);
- int loss_cnt = serial_omap_get_context_loss_count(up);
-
- if (loss_cnt 0) {
- dev_err(dev, serial_omap_get_context_loss_count failed : %d\n,
- loss_cnt);
- serial_omap_restore_context(up);
- } else if (up-context_loss_cnt != loss_cnt) {
- serial_omap_restore_context(up);
- }
+ serial_omap_restore_context(up);
up-latency = up-calc_latency;
schedule_work(up-qos_work);
--
1.8.1.rc1.5.g7e0651a
--
To unsubscribe from
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi,
On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
* Felipe Balbi ba...@ti.com [130214 03:19]:
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
Any comments to these last two sentences ?
I agree the sysconfig stuff should be handled in the drivers.
But considering that we need to also reset or idle hardware
that may not have a driver loaded, it's best to put the
reset/idle function in to the driver specific header as
an inline function.
that is likely to cause quite a few isues, don't you think?
hwmod depending on driver code to reset particular IPs ? Seems like we
need a generic device_reset() hook which can be called by platform code,
no ? Not sure if that would help a lot though.
This way the hwmod code can idle or reset the unclaimed
hardware in a late_initcall.
The problem with this:
my_driver.h:
static inline void my_driver_reset(struct my_driver *drv)
{
writel(drv-regs, SYSS, RESET);
}
how will hwmod provide drv pointer ? Even if we use void __iomem * it
would mean that hwmod would have to a) access driver's ioremaped area or
b) ioremap() the same area again.
Note sure if any of those are acceptable.
And probably the handling of sysconfig bits should be
done using a common header in include/linux/omap-hwmod.h
or something so it can be shared between drivers.
That could be done, though I think the header name could be different.
Maybe it could match the document which define those registers, let me
just check if we're allowed to use that publicly :-p
--
balbi
signature.asc
Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Felipe Balbi ba...@ti.com [130214 10:00]:
Hi,
On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
* Felipe Balbi ba...@ti.com [130214 03:19]:
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
Any comments to these last two sentences ?
Well I think all the driver handling should be done in
the driver since we now have runtime PM.
I agree the sysconfig stuff should be handled in the drivers.
But considering that we need to also reset or idle hardware
that may not have a driver loaded, it's best to put the
reset/idle function in to the driver specific header as
an inline function.
that is likely to cause quite a few isues, don't you think?
hwmod depending on driver code to reset particular IPs ? Seems like we
need a generic device_reset() hook which can be called by platform code,
no ? Not sure if that would help a lot though.
This way the hwmod code can idle or reset the unclaimed
hardware in a late_initcall.
The problem with this:
my_driver.h:
static inline void my_driver_reset(struct my_driver *drv)
{
writel(drv-regs, SYSS, RESET);
}
how will hwmod provide drv pointer ? Even if we use void __iomem * it
would mean that hwmod would have to a) access driver's ioremaped area or
b) ioremap() the same area again.
Well the ioremapping should only be done in the driver ideally.
Currently we have a mess of both hwmod and driver doing the
ioremapping.
So in the long run, let's assume only the driver ioremaps for
most of the cases where there is a driver available.
And only in the case there is no driver, hwmod can parse the address
space from DT for the unclaimed hardware in the late_initcall.
So the shared inline function should just take the __iomem * and
size instead of *drv so both the driver and hwmod code can tinker
with the autoidle bit.
Note sure if any of those are acceptable.
Hmm what issues do you see in the above suggestion?
And probably the handling of sysconfig bits should be
done using a common header in include/linux/omap-hwmod.h
or something so it can be shared between drivers.
That could be done, though I think the header name could be different.
Maybe it could match the document which define those registers, let me
just check if we're allowed to use that publicly :-p
Cool yeah I have no preference where that header should be. Maybe
something bus specific like include/linux/bus/omap-hwmod.h?
Regards,
Tony
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi,
On Thu, Feb 14, 2013 at 10:12:17AM -0800, Tony Lindgren wrote:
* Felipe Balbi ba...@ti.com [130214 10:00]:
Hi,
On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
* Felipe Balbi ba...@ti.com [130214 03:19]:
Currently the omap-serial driver will not
work properly if booted via DT with CPUIDLE
enabled because it depends on function pointers
provided by hwmod to change its own SYSCONFIG
register.
Remove that relyance on hwmod by moving SYSCONFIG
handling to driver itself. Note that this also
fixes a possible corner case bug where we could
be putting UART in Force Idle mode if we called
omap_serial_enable_wakeup(up, false) after setting
NOIDLE to the idle mode. This is because hwmod
has no protection against that situation.
Any comments to these last two sentences ?
Well I think all the driver handling should be done in
the driver since we now have runtime PM.
cool.
I agree the sysconfig stuff should be handled in the drivers.
But considering that we need to also reset or idle hardware
that may not have a driver loaded, it's best to put the
reset/idle function in to the driver specific header as
an inline function.
that is likely to cause quite a few isues, don't you think?
hwmod depending on driver code to reset particular IPs ? Seems like we
need a generic device_reset() hook which can be called by platform code,
no ? Not sure if that would help a lot though.
This way the hwmod code can idle or reset the unclaimed
hardware in a late_initcall.
The problem with this:
my_driver.h:
static inline void my_driver_reset(struct my_driver *drv)
{
writel(drv-regs, SYSS, RESET);
}
how will hwmod provide drv pointer ? Even if we use void __iomem * it
would mean that hwmod would have to a) access driver's ioremaped area or
b) ioremap() the same area again.
Well the ioremapping should only be done in the driver ideally.
Currently we have a mess of both hwmod and driver doing the
ioremapping.
right... and it only works because hwmod never calls
request_mem_region().
So in the long run, let's assume only the driver ioremaps for
most of the cases where there is a driver available.
makes sense.
And only in the case there is no driver, hwmod can parse the address
space from DT for the unclaimed hardware in the late_initcall.
sounds good. But then it means our DTS files should be 100% complete,
meaning that even for the IPs which we don't have drivers at all, we
should still provide DTS nodes.
In that case, does it make sense to teach DT about the actual structure
of OMAP's interconnect ? I mean:
ocp {
l3@ {
l4_per@ {
...
};
l4_wakeup@ {
...
};
...
};
};
That would mean that even interconnect PM could move to a driver under
drivers/bus/{l3,l4_per,l4_wakeup,..}.c
So the shared inline function should just take the __iomem * and
size instead of *drv so both the driver and hwmod code can tinker
with the autoidle bit.
Should hwmod be touching that in the long run ? It _does_ belong in the
device's address space
Note sure if any of those are acceptable.
Hmm what issues do you see in the above suggestion?
driver and hwmod accessing SYSC simultaneously for instance. Imagine
something like:
hwmod driver
-- ---
1. starts device reset
2. polls RESET bit with X ms timeoutX' ms later starts reset
3. times out since reset restarts polls RESET bit with X ms
timeout
4. error! reset completes
In such cases, what do we do ? There can be many such cases, don't
you think ?
And probably the handling of sysconfig bits should be
done using a common header in include/linux/omap-hwmod.h
or something so it can be shared between drivers.
That could be done, though I think the header name could be different.
Maybe it could match the document which define those registers, let me
just check if we're allowed to use that publicly :-p
Cool yeah I have no preference where that header should be. Maybe
something bus specific like include/linux/bus/omap-hwmod.h?
perhaps...
--
balbi
signature.asc
Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Felipe Balbi ba...@ti.com [130214 11:31]:
On Thu, Feb 14, 2013 at 10:12:17AM -0800, Tony Lindgren wrote:
And only in the case there is no driver, hwmod can parse the address
space from DT for the unclaimed hardware in the late_initcall.
sounds good. But then it means our DTS files should be 100% complete,
meaning that even for the IPs which we don't have drivers at all, we
should still provide DTS nodes.
Yes, eventually. It's still better to have only one set of that data
rather than number of supported SoCs times that data.. Parsing it
should not be too bad if it's done one driver at a time during the
driver probe.
In that case, does it make sense to teach DT about the actual structure
of OMAP's interconnect ? I mean:
ocp {
l3@ {
l4_per@ {
...
};
l4_wakeup@ {
...
};
...
};
};
That would mean that even interconnect PM could move to a driver under
drivers/bus/{l3,l4_per,l4_wakeup,..}.c
Yes makes sense to me.
So the shared inline function should just take the __iomem * and
size instead of *drv so both the driver and hwmod code can tinker
with the autoidle bit.
Should hwmod be touching that in the long run ? It _does_ belong in the
device's address space
Note sure if any of those are acceptable.
Hmm what issues do you see in the above suggestion?
driver and hwmod accessing SYSC simultaneously for instance. Imagine
something like:
hwmod driver
-----
1. starts device reset
2. polls RESET bit with X ms timeout X' ms later starts reset
3. times out since reset restarts polls RESET bit with X ms
timeout
4. error! reset completes
In such cases, what do we do ? There can be many such cases, don't
you think ?
I don't think so as hwmod should only touch the sysconfig space
when no driver has claimed it. If hwmod sysconfig tinkering is
only done in late_initcall, I don't think any drivers are probing
at that time? Well there may be some deferred probe related issues
there though, so we need some atomic way at that point to know if
some hardware is being claimed by a driver.
Regards,
Tony
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't have anything specifically to do with the underlying device IP block, but with the SoC integration, even though they live in the device's memory-mapped address range. It's unfortunate that TI didn't allocate a separate, small address space for these registers, translated by the bus, similar to the PCI-E Enhanced Configuration Access Mechanism: http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space ... but that's unfortunately the reality of the OMAP hardware. Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as long as the implementation of ioremap() can be overridden by the device's bus. PCI device drivers already do this -- albeit in a PCI-specific way -- with pci_ioremap_bar(): http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.gita=searchh=HEADst=greps=pci_ioremap_bar So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Any driver using such a mechanism would be intrinsically generic. platform_bus could simply set its bus-ioremap to the existing ioremap() function, while buses like a omap_hwmod_bus could use a function that returned the address range that omap_hwmod_bus has already ioremap()ped. regards, - Paul -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi,
On Thu, 14 Feb 2013, Paul Walmsley wrote:
So instead of something bus-specific like that, a better way would be to
use something like:
va = dev-bus-ioremap( ... );
va = dev-bus-iounmap( ... );
Something like this is what I was thinking. Obviously there would be more
patches needed, for the various arches, etc.; and I'm not convinced that
the function pointer init is done at the right time yet. Comments welcome.
- Paul
From: Paul Walmsley p...@pwsan.com
Date: Thu, 14 Feb 2013 13:49:58 -0700
Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
and iounmap()
On some hardware, such as OMAP, the bus abstraction code needs to call
ioremap(), since some SoC-integration registers are located in the
device address space. But generic device drivers should be able to
call ioremap() from driver code, for the majority of situations where
this isn't necessary. This experimental patch allows Linux bus abstraction
code to override all of the ioremap*() and iounmap() functions. In the OMAP
case, this would be used to return the previously-mapped address range from
the bus code, when called for the device's register area. This would avoid
a duplicate TLB mapping for that space.
This might also be useful as a generic replacement for pci_ioremap_bar().
Compile-tested only.
---
arch/arm/include/asm/io.h | 10 +-
arch/arm/mm/ioremap.c | 30 ++
arch/arm/mm/mmu.c |8
include/linux/device.h| 26 ++
4 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 652b560..22cc085 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -325,11 +325,11 @@ extern void _memset_io(volatile void __iomem *, int,
size_t);
* Documentation/io-mapping.txt.
*
*/
-#define ioremap(cookie,size) __arm_ioremap((cookie), (size),
MT_DEVICE)
-#define ioremap_nocache(cookie,size) __arm_ioremap((cookie), (size),
MT_DEVICE)
-#define ioremap_cached(cookie,size)__arm_ioremap((cookie), (size),
MT_DEVICE_CACHED)
-#define ioremap_wc(cookie,size)__arm_ioremap((cookie), (size),
MT_DEVICE_WC)
-#define iounmap__arm_iounmap
+extern void __iomem *ioremap(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_nocache(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_cached(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_wc(unsigned long cookie, size_t size);
+extern void iounmap(volatile void __iomem *va);
/*
* io{read,write}{8,16,32} macros
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 88fd86c..6507e69 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -398,3 +398,33 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t
phys_addr)
}
EXPORT_SYMBOL_GPL(pci_ioremap_io);
#endif
+
+void __iomem *ioremap(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap);
+
+void __iomem *ioremap_nocache(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap_nocache);
+
+void __iomem *ioremap_cached(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE_CACHED);
+}
+EXPORT_SYMBOL(ioremap_cached);
+
+void __iomem *ioremap_wc(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE_WC);
+}
+EXPORT_SYMBOL(ioremap_wc);
+
+void iounmap(volatile void __iomem *va)
+{
+ return __arm_iounmap(va);
+}
+EXPORT_SYMBOL(iounmap);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index ce328c7..303dba0 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -17,6 +17,7 @@
#include linux/fs.h
#include linux/vmalloc.h
#include linux/sizes.h
+#include linux/platform_device.h
#include asm/cp15.h
#include asm/cputype.h
@@ -28,6 +29,7 @@
#include asm/highmem.h
#include asm/system_info.h
#include asm/traps.h
+#include asm/io.h
#include asm/mach/arch.h
#include asm/mach/map.h
@@ -1246,4 +1248,10 @@ void __init paging_init(struct machine_desc *mdesc)
empty_zero_page = virt_to_page(zero_page);
__flush_dcache_page(NULL, empty_zero_page);
+
+ platform_bus_type.ioremap = ioremap;
+ platform_bus_type.ioremap_nocache = ioremap_nocache;
+ platform_bus_type.ioremap_cached = ioremap_cached;
+ platform_bus_type.ioremap_wc = ioremap_wc;
+ platform_bus_type.iounmap = iounmap;
}
diff --git a/include/linux/device.h b/include/linux/device.h
index 43dcda9..48c35e2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -71,6 +71,26 @@ extern void bus_remove_file(struct bus_type *, struct
bus_attribute *);
* @shutdown: Called at shut-down time to quiesce the device.
* @suspend: Called when a device on this bus wants to go to sleep mode.
* @resume:
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Thu, 14 Feb 2013, Paul Walmsley wrote: va = dev-bus-iounmap( ... ); And this should simply be dev-bus-iounmap( ... ); I regret the error... - Paul -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Paul Walmsley p...@pwsan.com [130214 12:51]: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 But that's triggered by runtime PM from the device driver, right? I think it's fine for hwmod to do that as requested by the device driver via runtime PM since hwmod is the bus glue making the drivers arch independent. I think the only piece we're missing for that is for driver to run something like pm_runtime_init() that initializes the address space to hwmod. Or just bus specific ioremap like you're suggesting later on. Just to recap what we've discussed earlier, the reasons why we want reset and idle functions should be in the driver specific header are: 1. There's often driver specific logic needed in addition to the syconfig tinkering in the reset/idle functions. 2. We need to be able to reset and idle some hardware even if the driver is not compiled in. Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't have anything specifically to do with the underlying device IP block, but with the SoC integration, even though they live in the device's memory-mapped address range. It's unfortunate that TI didn't allocate a separate, small address space for these registers, translated by the bus, similar to the PCI-E Enhanced Configuration Access Mechanism: http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space ... but that's unfortunately the reality of the OMAP hardware. Yes I think we all agree on accessing sysconfig in a standardized way via runtime PM triggered by the driver. Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as long as the implementation of ioremap() can be overridden by the device's bus. PCI device drivers already do this -- albeit in a PCI-specific way -- with pci_ioremap_bar(): http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.gita=searchh=HEADst=greps=pci_ioremap_bar So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Any driver using such a mechanism would be intrinsically generic. platform_bus could simply set its bus-ioremap to the existing ioremap() function, while buses like a omap_hwmod_bus could use a function that returned the address range that omap_hwmod_bus has already ioremap()ped. Yeah makes sense to me. Tony -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
* Paul Walmsley p...@pwsan.com [130214 13:44]:
Hi,
On Thu, 14 Feb 2013, Paul Walmsley wrote:
So instead of something bus-specific like that, a better way would be to
use something like:
va = dev-bus-ioremap( ... );
va = dev-bus-iounmap( ... );
Something like this is what I was thinking. Obviously there would be more
patches needed, for the various arches, etc.; and I'm not convinced that
the function pointer init is done at the right time yet. Comments welcome.
- Paul
From: Paul Walmsley p...@pwsan.com
Date: Thu, 14 Feb 2013 13:49:58 -0700
Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
and iounmap()
On some hardware, such as OMAP, the bus abstraction code needs to call
ioremap(), since some SoC-integration registers are located in the
device address space. But generic device drivers should be able to
call ioremap() from driver code, for the majority of situations where
this isn't necessary. This experimental patch allows Linux bus abstraction
code to override all of the ioremap*() and iounmap() functions. In the OMAP
case, this would be used to return the previously-mapped address range from
the bus code, when called for the device's register area. This would avoid
a duplicate TLB mapping for that space.
This might also be useful as a generic replacement for pci_ioremap_bar().
Compile-tested only.
The other option could be to allow custom ioremap function pointers
based on address range in __arm_ioremap_pfn_caller() the same way as
the SoC specific static mappings are allowed. That would require adding
a function pointer to struct map_desc.
Maybe that would do the trick?
Regards,
Tony
---
arch/arm/include/asm/io.h | 10 +-
arch/arm/mm/ioremap.c | 30 ++
arch/arm/mm/mmu.c |8
include/linux/device.h| 26 ++
4 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 652b560..22cc085 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -325,11 +325,11 @@ extern void _memset_io(volatile void __iomem *, int,
size_t);
* Documentation/io-mapping.txt.
*
*/
-#define ioremap(cookie,size) __arm_ioremap((cookie), (size),
MT_DEVICE)
j -#define ioremap_nocache(cookie,size)__arm_ioremap((cookie), (size),
MT_DEVICE)
-#define ioremap_cached(cookie,size) __arm_ioremap((cookie), (size),
MT_DEVICE_CACHED)
-#define ioremap_wc(cookie,size) __arm_ioremap((cookie), (size),
MT_DEVICE_WC)
-#define iounmap __arm_iounmap
+extern void __iomem *ioremap(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_nocache(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_cached(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_wc(unsigned long cookie, size_t size);
+extern void iounmap(volatile void __iomem *va);
/*
* io{read,write}{8,16,32} macros
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 88fd86c..6507e69 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -398,3 +398,33 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t
phys_addr)
}
EXPORT_SYMBOL_GPL(pci_ioremap_io);
#endif
+
+void __iomem *ioremap(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap);
+
+void __iomem *ioremap_nocache(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap_nocache);
+
+void __iomem *ioremap_cached(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE_CACHED);
+}
+EXPORT_SYMBOL(ioremap_cached);
+
+void __iomem *ioremap_wc(unsigned long cookie, size_t size)
+{
+ return __arm_ioremap(cookie, size, MT_DEVICE_WC);
+}
+EXPORT_SYMBOL(ioremap_wc);
+
+void iounmap(volatile void __iomem *va)
+{
+ return __arm_iounmap(va);
+}
+EXPORT_SYMBOL(iounmap);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index ce328c7..303dba0 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -17,6 +17,7 @@
#include linux/fs.h
#include linux/vmalloc.h
#include linux/sizes.h
+#include linux/platform_device.h
#include asm/cp15.h
#include asm/cputype.h
@@ -28,6 +29,7 @@
#include asm/highmem.h
#include asm/system_info.h
#include asm/traps.h
+#include asm/io.h
#include asm/mach/arch.h
#include asm/mach/map.h
@@ -1246,4 +1248,10 @@ void __init paging_init(struct machine_desc *mdesc)
empty_zero_page = virt_to_page(zero_page);
__flush_dcache_page(NULL, empty_zero_page);
+
+ platform_bus_type.ioremap = ioremap;
+ platform_bus_type.ioremap_nocache = ioremap_nocache;
+ platform_bus_type.ioremap_cached = ioremap_cached;
+ platform_bus_type.ioremap_wc =
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Thu, Feb 14, 2013 at 08:47:53PM +, Paul Walmsley wrote: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't why not ? It's part of the driver's address space anyway. It's not part of the IP in question (usb, ethernet, etc) but it's part of the TI wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the TI-specific integration registers (revision, sysc, syss...). So they're not part of the licensed IP, but they're part of the TI wrapper around those. have anything specifically to do with the underlying device IP block, but of course they do, soft reset touches the underlying IP, so does force idle, no idle, etc. But I agree that part is more changing the the asynchronous idle request handshake. with the SoC integration, even though they live in the device's memory-mapped address range. It's unfortunate that TI didn't allocate a separate, small address space for these registers, translated by the bus, similar to the PCI-E Enhanced Configuration Access Mechanism: http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space ... but that's unfortunately the reality of the OMAP hardware. right. Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as long as the implementation of ioremap() can be overridden by the device's bus. PCI device drivers already do this -- albeit in a PCI-specific way -- with pci_ioremap_bar(): http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.gita=searchh=HEADst=greps=pci_ioremap_bar So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Any driver using such a mechanism would be intrinsically generic. platform_bus could simply set its bus-ioremap to the existing ioremap() function, while buses like a omap_hwmod_bus could use a function that returned the address range that omap_hwmod_bus has already ioremap()ped. although I'm not sure I like the idea exposed here, ioremap() is the least of our problems :-) The biggest problem is having functioning PM for all drivers on a DT boot with CPUIDLE enabled. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Thu, Feb 14, 2013 at 02:47:10PM -0800, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 13:44]: Hi, On Thu, 14 Feb 2013, Paul Walmsley wrote: So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Something like this is what I was thinking. Obviously there would be more patches needed, for the various arches, etc.; and I'm not convinced that the function pointer init is done at the right time yet. Comments welcome. - Paul From: Paul Walmsley p...@pwsan.com Date: Thu, 14 Feb 2013 13:49:58 -0700 Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*() and iounmap() On some hardware, such as OMAP, the bus abstraction code needs to call ioremap(), since some SoC-integration registers are located in the device address space. But generic device drivers should be able to call ioremap() from driver code, for the majority of situations where this isn't necessary. This experimental patch allows Linux bus abstraction code to override all of the ioremap*() and iounmap() functions. In the OMAP case, this would be used to return the previously-mapped address range from the bus code, when called for the device's register area. This would avoid a duplicate TLB mapping for that space. This might also be useful as a generic replacement for pci_ioremap_bar(). Compile-tested only. The other option could be to allow custom ioremap function pointers based on address range in __arm_ioremap_pfn_caller() the same way as the SoC specific static mappings are allowed. That would require adding a function pointer to struct map_desc. Maybe that would do the trick? I'm not sure we should even try that. I mean, eventually (maybe in 100 years) we wouldn't mach-* at all and even struct map_desc would be dynamically initialized by data passed in via DTB, right ? Just consider Arnd's patch for the machine_desc-less DT boot. If we add a function pointer to struct map_desc, it will just become yet another function pointer to be removed later. -- balbi signature.asc Description: Digital signature
Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Thu, Feb 14, 2013 at 02:22:47PM -0800, Tony Lindgren wrote:
* Paul Walmsley p...@pwsan.com [130214 12:51]:
Hi,
On Thu, 14 Feb 2013, Tony Lindgren wrote:
I don't think so as hwmod should only touch the sysconfig space
when no driver has claimed it.
hwmod does need to touch the SYSCONFIG register during pm_runtime suspend
and resume operations, and during device enable and disable operations.
Here's the relevant code:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
But that's triggered by runtime PM from the device driver, right?
I think it's fine for hwmod to do that as requested by the device
driver via runtime PM since hwmod is the bus glue making the drivers arch
independent.
I think the only piece we're missing for that is for driver to run
something like pm_runtime_init() that initializes the address space
to hwmod. Or just bus specific ioremap like you're suggesting later
on.
Just to recap what we've discussed earlier, the reasons why we want
reset and idle functions should be in the driver specific header are:
1. There's often driver specific logic needed in addition to the
syconfig tinkering in the reset/idle functions.
how about introducing generic device_reset() and device_idle() hooks
which driver can implement and can be called by bus glue ?
Something like:
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 03d7bb1..9c921e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -256,6 +256,8 @@ typedef struct pm_message {
* these conditions and handle the device as appropriate, possibly queueing
* a suspend request for it. The return value is ignored by the PM core.
*
+ * @runtime_reset: Resets the device and puts it in a known state.
+ *
* Refer to Documentation/power/runtime_pm.txt for more information about the
* role of the above callbacks in device runtime power management.
*
@@ -285,6 +287,7 @@ struct dev_pm_ops {
int (*runtime_suspend)(struct device *dev);
int (*runtime_resume)(struct device *dev);
int (*runtime_idle)(struct device *dev);
+ int (*runtime_reset)(struct device *dev);
};
#ifdef CONFIG_PM_SLEEP
We already have -runtime_idle() which we could be using...
2. We need to be able to reset and idle some hardware even if the
driver is not compiled in.
yeah, this will be tricky. Even if you try late_initcall() there could
still be issues with -EPROBE_DEFER. Maybe you don't need to reset the
IPs but rather put those which have status = disabled to FORCE_IDLE.
Wouldn't that be enough ? If a driver claims the device later, then
pm_runtime_enable() + pm_runtime_get() will change that.
--
balbi
signature.asc
Description: Digital signature
RE: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
On Fri, Feb 15, 2013 at 12:23:08, Balbi, Felipe wrote:
On Thu, Feb 14, 2013 at 02:22:47PM -0800, Tony Lindgren wrote:
* Paul Walmsley p...@pwsan.com [130214 12:51]:
Hi,
On Thu, 14 Feb 2013, Tony Lindgren wrote:
I don't think so as hwmod should only touch the sysconfig space
when no driver has claimed it.
hwmod does need to touch the SYSCONFIG register during pm_runtime suspend
and resume operations, and during device enable and disable operations.
Here's the relevant code:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
But that's triggered by runtime PM from the device driver, right?
I think it's fine for hwmod to do that as requested by the device
driver via runtime PM since hwmod is the bus glue making the drivers arch
independent.
I think the only piece we're missing for that is for driver to run
something like pm_runtime_init() that initializes the address space
to hwmod. Or just bus specific ioremap like you're suggesting later
on.
Just to recap what we've discussed earlier, the reasons why we want
reset and idle functions should be in the driver specific header are:
1. There's often driver specific logic needed in addition to the
syconfig tinkering in the reset/idle functions.
how about introducing generic device_reset() and device_idle() hooks
which driver can implement and can be called by bus glue ?
Something like:
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 03d7bb1..9c921e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -256,6 +256,8 @@ typedef struct pm_message {
* these conditions and handle the device as appropriate, possibly queueing
* a suspend request for it. The return value is ignored by the PM core.
*
+ * @runtime_reset: Resets the device and puts it in a known state.
+ *
* Refer to Documentation/power/runtime_pm.txt for more information about the
* role of the above callbacks in device runtime power management.
*
@@ -285,6 +287,7 @@ struct dev_pm_ops {
int (*runtime_suspend)(struct device *dev);
int (*runtime_resume)(struct device *dev);
int (*runtime_idle)(struct device *dev);
+ int (*runtime_reset)(struct device *dev);
};
I am not a runtime PM expert but runtime_reset() looks a good option to me.
I expect most of the drivers won't need to do anything different from what
the hwmod code already does. IPs which have special reset requirements
can either extend the defaults ops or just override it. On AM33xx there
are some special requirements for CPSW, DCAN, PWMSS reset handling but
AFAIK none of the other IPs need to do anything special and we don't want
to duplicate the reset code in all of them.
Regards,
Vaibhav
--
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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
Hi, On Fri, Feb 15, 2013 at 12:14:29, Balbi, Felipe wrote: Hi, On Thu, Feb 14, 2013 at 08:47:53PM +, Paul Walmsley wrote: Hi, On Thu, 14 Feb 2013, Tony Lindgren wrote: I don't think so as hwmod should only touch the sysconfig space when no driver has claimed it. hwmod does need to touch the SYSCONFIG register during pm_runtime suspend and resume operations, and during device enable and disable operations. Here's the relevant code: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157 http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195 Drivers shouldn't touch their IP block's SYSCONFIG registers. They don't why not ? It's part of the driver's address space anyway. It's not part of the IP in question (usb, ethernet, etc) but it's part of the TI wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the TI-specific integration registers (revision, sysc, syss...). So they're not part of the licensed IP, but they're part of the TI wrapper around those. That's all the more reason to not allow the drivers to touch the SYSCONFIG register. We have IPs like EDMA, PWMSS, McASP etc which are common to Davinci and OMAP. The integration approach was different in the past and now if we want the same driver to work on both the platforms we have to keep the code for taking care of the integration details out of the drivers. IMO omap_hwmod does an excellent job of taking care of all or rather most of the IP integration idiosyncrasies. I really don't understand why people make a big fuss about hwmod. With the right SoC data things just work. Most of the driver authors don't take the pains to understand how the SoC PM gets impacted by toggling a few bits in SYSCONFIG and hence it's best to abstract away all the critical pieces out of drivers. For specific cases like custom reset handling I think it make much more sense to extend runtime PM that already build upon the hwmod code for OMAP. The drivers shouldn't have to worry about the integration details. Trying to shove a common piece of code into all the drivers is equivalent to taking a huge step backwards in the ongoing consolidation not just across OMAP and AMxx parts but also across Davinci and OMAP. Regards, Vaibhav -- 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/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency
+ Tero, Rajendra, Kevin On Friday 15 February 2013 12:16 PM, Felipe Balbi wrote: On Thu, Feb 14, 2013 at 02:47:10PM -0800, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [130214 13:44]: Hi, On Thu, 14 Feb 2013, Paul Walmsley wrote: So instead of something bus-specific like that, a better way would be to use something like: va = dev-bus-ioremap( ... ); va = dev-bus-iounmap( ... ); Something like this is what I was thinking. Obviously there would be more patches needed, for the various arches, etc.; and I'm not convinced that the function pointer init is done at the right time yet. Comments welcome. - Paul From: Paul Walmsley p...@pwsan.com Date: Thu, 14 Feb 2013 13:49:58 -0700 Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*() and iounmap() On some hardware, such as OMAP, the bus abstraction code needs to call ioremap(), since some SoC-integration registers are located in the device address space. But generic device drivers should be able to call ioremap() from driver code, for the majority of situations where this isn't necessary. This experimental patch allows Linux bus abstraction code to override all of the ioremap*() and iounmap() functions. In the OMAP case, this would be used to return the previously-mapped address range from the bus code, when called for the device's register area. This would avoid a duplicate TLB mapping for that space. This might also be useful as a generic replacement for pci_ioremap_bar(). Compile-tested only. The other option could be to allow custom ioremap function pointers based on address range in __arm_ioremap_pfn_caller() the same way as the SoC specific static mappings are allowed. That would require adding a function pointer to struct map_desc. Maybe that would do the trick? I'm not sure we should even try that. I mean, eventually (maybe in 100 years) we wouldn't mach-* at all and even struct map_desc would be dynamically initialized by data passed in via DTB, right ? Just consider Arnd's patch for the machine_desc-less DT boot. If we add a function pointer to struct map_desc, it will just become yet another function pointer to be removed later. On the same thread, it was also mentioned that, machine_desc-less DT boot is for simple machines. I have looked that aspect bit more. Considering the amount of callbacks OMAP uses, it is not practical. Ofcourse am also not in favor of adding another function pointer stuff here. We should get rid of dual ioremap() and the method suggested by Paul seems to be reasonable. Sysconfig handling is very tightly coupled with PRCM. I agree with Paul that hardware should have mapped it in separate address space. So whichever framework manages the PRCM, should also manage sysconfig. Refer all the issues around sysconfig for IP's like USB, UART and they are directly PRCM issues. We should be able to work around the IP integration issues around problematic IPs like UART, USB with some profile knowledge which can be handled by runtime PM transparently. It should have been done that way in first place instead of functions pointers which today break the Device Tree builds directly. As I mentioned in OMAP5 data series, we are now able to remove the IO_resource information ( Address space, IRQ data, DMA lines) from from hwmod data and extract it from device Tree. Thats a good point. Next one is getting rid off sysconfig function pointers which drivers are using and handle those issues using runtime_pm APIs. Idea is, IP gives profile like... e.g McSPI1 - Smart Idle always UART - no_idle(runtime_get) and smart_idle(runtime_put) mUSB -- no_idle(runtime_get) and force_idle(runtime_put) Then the last one is custom reset function for broken IPs. For this one as well if we can add a hook in runtime framework, then it can be handled without direct function calls. Regards, Santosh Regards Santosh -- 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
