Re: hwmod: multi-omap: disabling smartreflex on AM3517

["Cousson, Benoit" <b-cousson@ti.com>]

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.
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.

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);
...

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