| Message ID | 50FD5993.5010008@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Santosh Shilimkar <santosh.shilimkar@ti.com> [130121 07:09]: > > > So I looked at this one with help of Rajendra. We can get rid of the > IRQ and DMA data(needs DMA biding updates) easily. The address > space though is needed since hwmod code uses it to setup the > sysconfig registers. OK great. The address space tinkering in hwmod code should be moved to be done in the drivers. As discussed earlier, there should be a driver specific reset function driver_xyz_reset() in the driver header file so the hwmod code can call it too in a late_initcall if no driver is loaded. > Extracting that from DT code seems to be really expensive and > ugly [1]. I am yet to try out DMA lines removal but that seems > to be doable by pulling Vinod'd DMA engine branch and updating > DT file. The overhead here does not matter as it should only happen in a late_initcall and only for some of the drivers. For that to happen we just need to go through the list of modules not yet probed. We also need to have some locking in the driver specific reset function to avoid races with the loadable modules. > Whats your suggestion on address space part ? Let's add the code to hwmod to extract it from DT so hwmod code can call the driver specific reset function defined in the driver header. That way we can start moving the driver code out of hwmod one driver at a time. Note that the ioremapping should be done in the driver specific reset function, not in hwmod code. We just need to pass the iorange in a struct resource to the driver specific reset function. Regards, Tony
Hi Tony, On 01/21/2013 07:01 PM, Tony Lindgren wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130121 07:09]: >>> >> So I looked at this one with help of Rajendra. We can get rid of the >> IRQ and DMA data(needs DMA biding updates) easily. The address >> space though is needed since hwmod code uses it to setup the >> sysconfig registers. > > OK great. The address space tinkering in hwmod code should be > moved to be done in the drivers. > > As discussed earlier, there should be a driver specific reset > function driver_xyz_reset() in the driver header file so the > hwmod code can call it too in a late_initcall if no driver is > loaded. > >> Extracting that from DT code seems to be really expensive and >> ugly [1]. I am yet to try out DMA lines removal but that seems >> to be doable by pulling Vinod'd DMA engine branch and updating >> DT file. > > The overhead here does not matter as it should only happen in a > late_initcall and only for some of the drivers. For that to > happen we just need to go through the list of modules not yet > probed. We also need to have some locking in the driver specific > reset function to avoid races with the loadable modules. Mmm, not really, that address is used by *every* hwmod for sysconfig access. So iterating over every DT nodes for every hwmods seems pretty ugly and un-optimized. That being said, it might worth checking the overhead. That will not make the fix nicer anyway, but at least it will allow a smooth transition toward a real clean solution. Assuming someone will work on that later, which might never happen. Regards, Benoit
Hi Tony, >> So I looked at this one with help of Rajendra. We can get rid of the >> IRQ and DMA data(needs DMA biding updates) easily. The address >> space though is needed since hwmod code uses it to setup the >> sysconfig registers. > > OK great. The address space tinkering in hwmod code should be > moved to be done in the drivers. > > As discussed earlier, there should be a driver specific reset > function driver_xyz_reset() in the driver header file so the > hwmod code can call it too in a late_initcall if no driver is > loaded. I am a little confused with what you are saying. The hwmod doing reset of modules (and not relying on drivers doing it) was mainly for modules which do not have drivers built in (and hence run a risk of gating system sleep in case the bootloaders left them in a bad state). But if the drivers aren't built in (or are built as modules) *then* hwmod still needs to be able to do reset (maybe in a late_initcall) of these modules on its own (because there is no driver code to do it). The other big reason why hwmod would need the address space tinkering is because it also controls the various OCP master/slave modes of these modules. Quite often these modes are broken and need tinkering every time the module is enable/idled and also need to be restored back to sane values (smart_idle/smart_standby) post reset. All of this is today handled as part of hwmod and would defeat the whole purpose of the framework if all this is moved into drivers. So completely getting rid of all address space tinkering in hwmod looks really difficult. regards, Rajendra > >> Extracting that from DT code seems to be really expensive and >> ugly [1]. I am yet to try out DMA lines removal but that seems >> to be doable by pulling Vinod'd DMA engine branch and updating >> DT file. > > The overhead here does not matter as it should only happen in a > late_initcall and only for some of the drivers. For that to > happen we just need to go through the list of modules not yet > probed. We also need to have some locking in the driver specific > reset function to avoid races with the loadable modules. > >> Whats your suggestion on address space part ? > > Let's add the code to hwmod to extract it from DT so hwmod code > can call the driver specific reset function defined in the driver > header. That way we can start moving the driver code out of hwmod > one driver at a time. > > Note that the ioremapping should be done in the driver specific > reset function, not in hwmod code. We just need to pass the > iorange in a struct resource to the driver specific reset function. > > Regards, > > Tony >
* Benoit Cousson <b-cousson@ti.com> [130122 04:59]: > Hi Tony, > > On 01/21/2013 07:01 PM, Tony Lindgren wrote: > > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130121 07:09]: > >>> > >> So I looked at this one with help of Rajendra. We can get rid of the > >> IRQ and DMA data(needs DMA biding updates) easily. The address > >> space though is needed since hwmod code uses it to setup the > >> sysconfig registers. > > > > OK great. The address space tinkering in hwmod code should be > > moved to be done in the drivers. > > > > As discussed earlier, there should be a driver specific reset > > function driver_xyz_reset() in the driver header file so the > > hwmod code can call it too in a late_initcall if no driver is > > loaded. > > > >> Extracting that from DT code seems to be really expensive and > >> ugly [1]. I am yet to try out DMA lines removal but that seems > >> to be doable by pulling Vinod'd DMA engine branch and updating > >> DT file. > > > > The overhead here does not matter as it should only happen in a > > late_initcall and only for some of the drivers. For that to > > happen we just need to go through the list of modules not yet > > probed. We also need to have some locking in the driver specific > > reset function to avoid races with the loadable modules. > > Mmm, not really, that address is used by *every* hwmod for sysconfig > access. So iterating over every DT nodes for every hwmods seems pretty > ugly and un-optimized. I think you missed one point. Iterating over all the modules should only happen for the unused modules. With sysconfig access moved to the drivers getting the ioaddress is done in the standard way in the driver probe instead of iterating over all of them. > That being said, it might worth checking the overhead. That will not > make the fix nicer anyway, but at least it will allow a smooth > transition toward a real clean solution. Assuming someone will work on > that later, which might never happen. Right, so who is going to do all the work needed? Regards, Tony
* Rajendra Nayak <rnayak@ti.com> [130122 05:17]: > Hi Tony, > > >>So I looked at this one with help of Rajendra. We can get rid of the > >>IRQ and DMA data(needs DMA biding updates) easily. The address > >>space though is needed since hwmod code uses it to setup the > >>sysconfig registers. > > > >OK great. The address space tinkering in hwmod code should be > >moved to be done in the drivers. > > > >As discussed earlier, there should be a driver specific reset > >function driver_xyz_reset() in the driver header file so the > >hwmod code can call it too in a late_initcall if no driver is > >loaded. > > I am a little confused with what you are saying. The hwmod doing > reset of modules (and not relying on drivers doing it) was > mainly for modules which do not have drivers built in (and hence > run a risk of gating system sleep in case the bootloaders left > them in a bad state). Right, but we should not duplicate driver code in the hwmod in those cases. And that's why those reset functions need to be in the driver specific headers so they work both for the driver probe, and hwmod late_initcall. > But if the drivers aren't built in (or are built as modules) *then* > hwmod still needs to be able to do reset (maybe in a late_initcall) of > these modules on its own (because there is no driver code to do it). Yes that's the idea. > The other big reason why hwmod would need the address space > tinkering is because it also controls the various OCP master/slave > modes of these modules. Quite often these modes are broken and > need tinkering every time the module is enable/idled and also > need to be restored back to sane values (smart_idle/smart_standby) > post reset. All of this is today handled as part of hwmod and > would defeat the whole purpose of the framework if all this is > moved into drivers. It seems that we should have the iospace available in the driver at that point. And it should be also available to the runtime PM code in hwmod in the device somewhere? > So completely getting rid of all address space tinkering in hwmod > looks really difficult. We can certainly use common hwmod functions via runtime PM for doing that. There should not be any need to ioremap things both in hwmod and in the driver. And there certainly should not be need to provide duplicate iospace from both DT and hwmod. As the DT is what we're standardizing on, we really must move over to use that data. Regards, Tony
On Wednesday 23 January 2013 12:09 AM, Tony Lindgren wrote: > It seems that we should have the iospace available in the driver > at that point. And it should be also available to the runtime PM > code in hwmod in the device somewhere? I couldn't find anything as part of the device. Definitely not the ioremaped address. There's a device_private->driver_data which the drivers can use to populate the ioremaped address but I am sure thats not whats its meant for.
* Rajendra Nayak <rnayak@ti.com> [130124 01:54]: > On Wednesday 23 January 2013 12:09 AM, Tony Lindgren wrote: > >It seems that we should have the iospace available in the driver > >at that point. And it should be also available to the runtime PM > >code in hwmod in the device somewhere? > > I couldn't find anything as part of the device. Definitely not the > ioremaped address. There's a device_private->driver_data which the > drivers can use to populate the ioremaped address but I am sure thats > not whats its meant for. I guess we could have runtime PM init function to pass the driver region to the low level bus code if needed. Anybody got better ideas? Regards, Tony
On Wednesday 23 January 2013 12:02 AM, Tony Lindgren wrote: > * Benoit Cousson <b-cousson@ti.com> [130122 04:59]: >> Hi Tony, >> >> On 01/21/2013 07:01 PM, Tony Lindgren wrote: >>> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130121 07:09]: >>>>> >>>> So I looked at this one with help of Rajendra. We can get rid of the >>>> IRQ and DMA data(needs DMA biding updates) easily. The address >>>> space though is needed since hwmod code uses it to setup the >>>> sysconfig registers. >>> >>> OK great. The address space tinkering in hwmod code should be >>> moved to be done in the drivers. >>> >>> As discussed earlier, there should be a driver specific reset >>> function driver_xyz_reset() in the driver header file so the >>> hwmod code can call it too in a late_initcall if no driver is >>> loaded. >>> >>>> Extracting that from DT code seems to be really expensive and >>>> ugly [1]. I am yet to try out DMA lines removal but that seems >>>> to be doable by pulling Vinod'd DMA engine branch and updating >>>> DT file. >>> >>> The overhead here does not matter as it should only happen in a >>> late_initcall and only for some of the drivers. For that to >>> happen we just need to go through the list of modules not yet >>> probed. We also need to have some locking in the driver specific >>> reset function to avoid races with the loadable modules. >> >> Mmm, not really, that address is used by *every* hwmod for sysconfig >> access. So iterating over every DT nodes for every hwmods seems pretty >> ugly and un-optimized. > > I think you missed one point. Iterating over all the modules should > only happen for the unused modules. With sysconfig access moved to the > drivers getting the ioaddress is done in the standard way in the driver > probe instead of iterating over all of them. > >> That being said, it might worth checking the overhead. That will not >> make the fix nicer anyway, but at least it will allow a smooth >> transition toward a real clean solution. Assuming someone will work on >> that later, which might never happen. > > Right, so who is going to do all the work needed? > OK so we do managed to clean up the address space, IRQ lines and DMA request lines data from hwmod completely. -OMAP5 hwmod data file, 2076 lines we could remove which significant reduction. I ran the same scripts on OMAP4 and there too about 2200 lines getting deleted. - I have to udapte DT file to add the all supported hwmods with reg property so that OMAP5 continue to boot. Similar work is needed for OMAP4 too once OMAP4 is made DT only support. - To my suprise, the DT lookup isn't that bad. It is adding just 24 milliseconds to the boot time which is more or less noise. Have pushed a branch with above update for OMAP5 here [1] So we are left with two other topics which you mentioned in the comments. 1. Movement of clock data to drivers/clk. Till we get direction here I would like to hear the alternative to get OMAP5 booting from mainline. If there is no alternative, we can keep OMAP5 clock data alone out of tree and get rest of the data files merged. 2. The iormap() done by hwmod for sysconfig handling which you are discussing with Rajendra. So far we don't have a viable way to get the iormapped address from device drivers back to platform code. Lets continue on this thread but this can evolve in parallel. Let me know your thoughts. Regards, Santosh [1] git://github.com/SantoshShilimkar/linux.git 3.9/omap5-testing-hwmod-cleanup
* Santosh Shilimkar <santosh.shilimkar@ti.com> [130129 05:59]: > OK so we do managed to clean up the address space, IRQ lines > and DMA request lines data from hwmod completely. > > -OMAP5 hwmod data file, 2076 lines we could remove which significant > reduction. I ran the same scripts on OMAP4 and there too about 2200 > lines getting deleted. Great, thanks for looking into it. I guess we cannot do that quite yet for omap4 as we have not made it DT only. But we should be able to do that for am33xx as that's DT only already. > - I have to udapte DT file to add the all supported hwmods with reg > property so that OMAP5 continue to boot. Similar work is needed for > OMAP4 too once OMAP4 is made DT only support. OK > - To my suprise, the DT lookup isn't that bad. It is adding just > 24 milliseconds to the boot time which is more or less noise. That's good to hear. > Have pushed a branch with above update for OMAP5 here [1] > > So we are left with two other topics which you mentioned in the > comments. > > 1. Movement of clock data to drivers/clk. Till we get direction here > I would like to hear the alternative to get OMAP5 booting from mainline. > If there is no alternative, we can keep OMAP5 clock data alone > out of tree and get rest of the data files merged. I agree, no reason to hold back the other patches. But we should resolve the common clock move to drivers/clk properly now, otherwise it will just get postponed again and we have even bigger problem to deal with. > 2. The iormap() done by hwmod for sysconfig handling which you are > discussing with Rajendra. So far we don't have a viable way to > get the iormapped address from device drivers back to platform > code. Lets continue on this thread but this can evolve in > parallel. Yes that can be fixed separately. Regards, Tony > [1] git://github.com/SantoshShilimkar/linux.git > 3.9/omap5-testing-hwmod-cleanup
On Tuesday 29 January 2013 10:54 PM, Tony Lindgren wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130129 05:59]: >> OK so we do managed to clean up the address space, IRQ lines >> and DMA request lines data from hwmod completely. >> >> -OMAP5 hwmod data file, 2076 lines we could remove which significant >> reduction. I ran the same scripts on OMAP4 and there too about 2200 >> lines getting deleted. > > Great, thanks for looking into it. I guess we cannot do that quite > yet for omap4 as we have not made it DT only. But we should be able > to do that for am33xx as that's DT only already. > >> - I have to udapte DT file to add the all supported hwmods with reg >> property so that OMAP5 continue to boot. Similar work is needed for >> OMAP4 too once OMAP4 is made DT only support. > > OK > >> - To my suprise, the DT lookup isn't that bad. It is adding just >> 24 milliseconds to the boot time which is more or less noise. > > That's good to hear. > >> Have pushed a branch with above update for OMAP5 here [1] >> >> So we are left with two other topics which you mentioned in the >> comments. >> >> 1. Movement of clock data to drivers/clk. Till we get direction here >> I would like to hear the alternative to get OMAP5 booting from mainline. >> If there is no alternative, we can keep OMAP5 clock data alone >> out of tree and get rest of the data files merged. > > I agree, no reason to hold back the other patches. But we should > resolve the common clock move to drivers/clk properly now, > otherwise it will just get postponed again and we have even bigger > problem to deal with. > I will go ahead and separate the clock data from rest of the data files so that we can get rest of the data patches in. >> 2. The iormap() done by hwmod for sysconfig handling which you are >> discussing with Rajendra. So far we don't have a viable way to >> get the iormapped address from device drivers back to platform >> code. Lets continue on this thread but this can evolve in >> parallel. > > Yes that can be fixed separately. > Yep. Thanks for the comments. Regards Santosh
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 4653efb..f54b9d4 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -138,6 +138,7 @@ #include <linux/spinlock.h> #include <linux/slab.h> #include <linux/bootmem.h> +#include <linux/of.h> #include "clock.h" #include "omap_hwmod.h" @@ -2335,7 +2336,12 @@ static int _shutdown(struct omap_hwmod *oh) static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data) { struct omap_hwmod_addr_space *mem; - void __iomem *va_start; + void __iomem *va_start = NULL; + struct device_node *np; + unsigned long start = 0, size = 0; + const void *reg_prop; + const char *p; + if (!oh) return; @@ -2349,15 +2355,32 @@ static void __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data) if (!mem) { pr_debug("omap_hwmod: %s: no MPU register target found\n", oh->name); - return; + /*Check in Device Tree blob*/ + for_each_child_of_node(of_find_node_by_name(NULL, "ocp"), np) { + printk("np-name=%s\n", np->name); + if(of_find_property(np, "ti,hwmods", NULL)) { + p = of_get_property(np, "ti,hwmods", NULL); + if (!strcmp(p, oh->name)) { + reg_prop = of_get_property(np, "reg", NULL); + start = of_read_number(reg_prop, 1); + size = of_read_number(reg_prop + 4, 1); + } + } + } + } else { + start = mem->pa_start; + size = mem->pa_end - mem->pa_start; } - va_start = ioremap(mem->pa_start, mem->pa_end - mem->pa_start); + if (!start) + return; + + va_start = ioremap(start, size); if (!va_start) { pr_err("omap_hwmod: %s: Could not ioremap\n", oh->name); return; } - + pr_debug("omap_hwmod: %s: MPU register target at va %p\n", oh->name, va_start);