>From: Cousson, Benoit
>Sent: Wednesday, February 23, 2011 4:23 AM
>To: Premi, Sanjeev
>Cc: [email protected]; 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
