> -----Original Message-----
> From: Cousson, Benoit
> Sent: Wednesday, February 23, 2011 9:23 PM
> To: Premi, Sanjeev
> Cc: linux-omap@xxxxxxxxxxxxxxx; Paul Walmsley
> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
>
> Hi Sanjeev,
>
> On 2/23/2011 9:46 AM, Premi, Sanjeev wrote:
> >> 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.
>
> Yes, I did. But this is not really different than this email, isn't it?
> You still have to tag the hwmod you'd like to exclude using a new field.
>
> >> 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?
>
> No, but voltage domain will not be handled by hwmod anyway, so it will
> have to go to the voltage domain data file using some similar mechanism.
>
> > 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.
>
> In that case you can handle that using cpu_is_3517().
>
> omap_hwmod_init(omap3xxx_common_hwmods);
> if (omap3_has_smartreflex())
> omap_hwmod_init(omap3xxx_sr_hwmods);
> if (omap3_has_iva())
> omap_hwmod_init(omap3xxx_iva_hwmods);
> if (cpu_is_omap3517())
> omap_hwmod_init(omap3xxx_emac_usb_vpfe_hwmods);
> ...
>
> Isn't it enough for your needs?
>
[sp] Removal of smartreflex has more cyclic dependencies than I expected.
Just removing omap34xx_sr1_hwmod, omap34xx_sr2_hwmod isn't sufficient.
It leads to:
[ 0.000000] omap_hwmod: l4_core: cannot clk_get interface_clk sr_l4_ick
[ 0.000000] omap_hwmod: l4_core: cannot clk_get interface_clk sr_l4_ick
Now wemoving, omap3_l4_core__sr1 and omap3_l4_core__sr2 from the array
omap3xxx_l4_core_slaves[] leads to ripple effect in pretty much all
hwmods.
. omap3xxx_l4_core_slaves is referenced in omap3xxx_l4_core_hwmod,
. omap3xxx_l4_core_hwmod is referenced in many other hdmods.
. and so on.
In the end, we might end up making changes to almost all.
I will be submitting an updated patch in about an hour.
~sanjeev
> Ideally we should not even have all these functions, we should try to
> maintain a table with CHIP / FEATURE information to allow detection of
> hwmod_list to include / exclude.
>
> 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
