>From: Cousson, Benoit >Sent: Wednesday, February 23, 2011 4:23 AM >To: Premi, Sanjeev >Cc: linux-omap@xxxxxxxxxxxxxxx; Paul Walmsley >Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517 > >On 2/22/2011 2:48 PM, Premi, Sanjeev wrote: >> On 2/21/2011 4:41 PM, Premi, Sanjeev wrote: >>> >>> [...] >>> >>>>> The comment is already there BTW, so you just have to replace that by some >>>>> real code:-) >>>> >>>> [sp] I have already added real code, but the problem lies here: >>>> On same file (few lines up) omap_chip.oc is assigned value of >>>> CHIP_IS_OMAP3430. CHIP_IS_AM3517 now needs to be added to all >>>> places where CHIP_IS_OMAP3430ES3_1 is chosen. >>>> >>>> All this to support a chip that differs in 4 peripherals and IVA. >>>> ... and this is what I was planning to minimize. >>> >>> This is what we've being using for some time to handle small diff >>> between ES. >>> >>>> Leaving aside AM3517; we have AM3703 - same as OMAP3630 but without >>>> IVA and SGX. Here obviously hwmods for either of IVA, SGX shouldn't >>>> be initialized. Isn't it? >>>> >>>> Creating CHIP_IS_ ... here would be an overkill. Thoughts? >>> >>> It depends how many variant you plan to do :-) We still have some room >>> for 18 more variants / chip. >>> >>> You can still create a new CHIP_IS, and add a alias >>> CHIP_IS_OMAP36XX_COMMON = CHIP_IS_OMAP3630 | CHIP_IS_AM3703 and then >>> replace all the existing entry with that alias. >> >> [sp] This means we would need 5 bits for AM37xx devices. Adding another >> 5 bits for OMAP35xx and 3 bits for AM35xx device, we are left with >> only 5. (Assuming no further silicon revisions across these devices) >> >> We will be short of bits when support for TI814x variants come in. >> Also, there are going to be too many ORs for the HWMODs that are >> reused across the ARM Cortex-A8 family. >> >> In my previous mail, I was proposing simpler scheme where default >> definition is "inclusive" as now; and then ones not applicable for >> each specific device is simply "excluded" from consideration before >> HWMODs are initialized. >> >>> >>> If we want to avoid using these defines, you will have potentially to >>> add a feature entry in every hwmod / clock / power domain entry that >>> already uses the omap_chip today. >>> And then during the init we can filter on both the chip revision and >>> chip features. >>> The drawback is that we are going to waste at least 300 x 32 bits to >>> store that :-) >>> Whereas with the extra CHIP_IS_, it is just a couple of defines... no >>> memory impact. >> >> [sp] It is not just one additional bit; but 1 bit for the family e.g. >> CHIP_IS_OMAP36XX_COMMON; and then one per variant. >> >> Much less than 300x32 but still lot of code changes compared to >> actual difference between the devices. >> >>> >>> In between, you can consider a hwmod class to feature mapping, in order >>> to know what hwmod class will be excluded if the feature is not there >>> during the iteration. >> >> [sp] Here is what I had been proposing with one hwmod as example. >> >> static struct omap_hwmod omap34xx_sr1_hwmod = { >> .name = "sr1_hwmod", >> >> ...removed much of code... >> >> .select = true; /* new flag. True by default. */ >> }; >> >> Later: >> >> int __init omap3xxx_hwmod_init(void) >> { >> if (cpu_is_omap3505()) { >> omap3505_hwmod_select(); >> } >> else if (cpu_is_omap3517()) { >> omap3505_hwmod_select(); >> } >> else if (cpu_is_omap34xx()) { >> if (cpu_is_omap3503()) >> omap3503_hwmod_select(); >> else if (cpu_is_omap3515()) >> omap3515_hwmod_select(); >> } >> ...and so on... >> >> return omap_hwmod_init(omap3xxx_hwmods); >> } >> >> where, >> >> void __init omap3505_hwmod_select(void) >> { >> omap34xx_sr1_hwmod.select = false; >> omap34xx_sr2_hwmod.select = false; >> >> ... etc. etc. ... >> } >> >> And the omap_hwmod_init() first checks for .select to be true. >> Current processing works as is. >> >> This way, we don't need additional bits per chip variant; and >> we reuse the existing "feature" bits. > >Since you have to add some extra fields anyway, I'd rather add a full >feature entry. >It will avoid adding all this code and it will be much more scalable. >Here you have to update the code and add an extra function for every new >variant. [sp] In reality, if we go this route, we don't really need any other field. for this purpose. Once the required HWMODs are selected at init time, we shouldn't need any runtime check for applicability - via omap_chip OR otherwise - at runtime; but there are other implications so i would rather leave it there for now. Have you looked at the RFC i submitted yesterday - that illustrates the change better. >Do not forget that since OMAP4 we are generating this file, that's why I >clearly prefer to rely on data information rather that some function. [sp] This doesn't impact/chhange the auto generation. Because auto-generated code can have the select field true by default. I am still to understand the differences between OMAP4430 and OMAP4440, but wouldn't it be easy to handle the "selection" process in a function. Of course, it depends upon how the generation is being done. > >Or, we can potentially consider building severals omapxxxx_hwmods lists >and init only the relevant one based on features. > >omap_hwmod_init(omap3xxx_hwmods); >if (omap3_has_smartreflex()) > omap_hwmod_init(omap3xxx_sr_hwmods); >if (omap3_has_iva()) > omap_hwmod_init(omap3xxx_iva_hwmods); [sp] So now let us look at differences between AM3517 and OMAP35x. 1) No smart reflex, No IVA in AM3517. => Easily handled by feature as in illustration above. 2) Voltage domains are collapsed in AM3517. No scalability. => Does it really merit to be called feature? 3) These IPs are different - EMAC, USB, VPFE - coming from the DaVinci processor series. => Again, they are not features. We can go overboard and make them one viz. davinci_emac, davinci_usb, ... But, would this really scale up as more peripherals are added in each of the AM35x, AM37x and TI816x series. ~sanjeev >... > >This is still not super scalable for my point of view, but much easier >to handle and update. The advantage being, it will not increase the >memory usage at all. >The omap_hwmod_init will just have to be slightly updated to allow >multiple init. > >Regards, >Benoit >-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html