hwmod: multi-omap: disabling smartreflex on AM3517

hwmod: multi-omap: disabling smartreflex on AM3517

[Premi, Sanjeev]

AM3517 doesn't support SmartReflex.

However, these HWMODS are defined in omap3xxxx_hwmods[]:
	&omap34xx_sr1_hwmod,
	&omap34xx_sr2_hwmod,
	&omap36xx_sr1_hwmod,
	&omap36xx_sr2_hwmod,

(similar definition in l4_slaves as well)

This leads to crash when booting the kernel on AM3517EVM during
_setup().

I see the field .omap_chip being initialized; but not used.

If I were to use this - along with cpu_is_omap3517(), I would need
to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
devices defined in omap_hwmod_3xxx_data.c.

Before going all out on making changes, wanted to check if there is
a better way. Has this/similar possibility been considered earlier?

Best regards,
Sanjeev

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

[Cousson, Benoit]

Hi Sanjeev,

On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
> AM3517 doesn't support SmartReflex.
>
> However, these HWMODS are defined in omap3xxxx_hwmods[]:
> 	&omap34xx_sr1_hwmod,
> 	&omap34xx_sr2_hwmod,
> 	&omap36xx_sr1_hwmod,
> 	&omap36xx_sr2_hwmod,
>
> (similar definition in l4_slaves as well)
>
> This leads to crash when booting the kernel on AM3517EVM during
> _setup().
>
> I see the field .omap_chip being initialized; but not used.

Yes, it is. During the hwmod initialization (omap_hwmod_init), only the 
hwmods that match the correct chip version are kept.
I guess that your problem is that AM3517 is probably seen as a regular 
3430 for the moment.

> If I were to use this - along with cpu_is_omap3517(), I would need
> to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
> devices defined in omap_hwmod_3xxx_data.c.
>
> Before going all out on making changes, wanted to check if there is
> a better way. Has this/similar possibility been considered earlier?

Well, this is the best way to do that for my point of view. This 
.omap_chip field was done for that purpose.
During device init, the sr code will do query for the smartreflex hwmod 
and will failed, thus avoiding to do further initialization.

Regards,
Benoit

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

[Premi, Sanjeev]

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Tuesday, February 15, 2011 6:18 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> 
> Hi Sanjeev,
> 
> On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
> > AM3517 doesn't support SmartReflex.
> >
> > However, these HWMODS are defined in omap3xxxx_hwmods[]:
> > 	&omap34xx_sr1_hwmod,
> > 	&omap34xx_sr2_hwmod,
> > 	&omap36xx_sr1_hwmod,
> > 	&omap36xx_sr2_hwmod,
> >
> > (similar definition in l4_slaves as well)
> >
> > This leads to crash when booting the kernel on AM3517EVM during
> > _setup().
> >
> > I see the field .omap_chip being initialized; but not used.
> 
> Yes, it is. During the hwmod initialization (omap_hwmod_init), only the
> hwmods that match the correct chip version are kept.
> I guess that your problem is that AM3517 is probably seen as a regular
> 3430 for the moment.
> 
> > If I were to use this - along with cpu_is_omap3517(), I would need
> > to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
> > devices defined in omap_hwmod_3xxx_data.c.
> >
> > Before going all out on making changes, wanted to check if there is
> > a better way. Has this/similar possibility been considered earlier?
> 
> Well, this is the best way to do that for my point of view. This
> .omap_chip field was done for that purpose.
> During device init, the sr code will do query for the smartreflex hwmod
> and will failed, thus avoiding to do further initialization.

[sp] Trying to avoid big change, and thinking 'narrowly' about this
     issue in isolation, I had been mulling adding SmartReflex to
     the omap3_features; and (somehow) using the same.

     But after noticing the patch related to USBOTG on AM35x, I think
     original proposal is unambiguous and best way forward.

     Started working on the patch. Hope to have it ready later tonight
     or tomorrow.

~sanjeev

> 
> Regards,
> Benoit

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

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Friday, February 18, 2011 5:43 PM
> To: Cousson, Benoit
> Cc: linux-omap@vger.kernel.org
> Subject: RE: hwmod: multi-omap: disabling smartreflex on AM3517
> 
> > -----Original Message-----
> > From: Cousson, Benoit
> > Sent: Tuesday, February 15, 2011 6:18 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> >
> > Hi Sanjeev,
> >
> > On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
> > > AM3517 doesn't support SmartReflex.
> > >
> > > However, these HWMODS are defined in omap3xxxx_hwmods[]:
> > > 	&omap34xx_sr1_hwmod,
> > > 	&omap34xx_sr2_hwmod,
> > > 	&omap36xx_sr1_hwmod,
> > > 	&omap36xx_sr2_hwmod,
> > >
> > > (similar definition in l4_slaves as well)
> > >
> > > This leads to crash when booting the kernel on AM3517EVM during
> > > _setup().
> > >
> > > I see the field .omap_chip being initialized; but not used.
> >
> > Yes, it is. During the hwmod initialization (omap_hwmod_init), only the
> > hwmods that match the correct chip version are kept.
> > I guess that your problem is that AM3517 is probably seen as a regular
> > 3430 for the moment.
> >
> > > If I were to use this - along with cpu_is_omap3517(), I would need
> > > to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
> > > devices defined in omap_hwmod_3xxx_data.c.
> > >
> > > Before going all out on making changes, wanted to check if there is
> > > a better way. Has this/similar possibility been considered earlier?
> >
> > Well, this is the best way to do that for my point of view. This
> > .omap_chip field was done for that purpose.
> > During device init, the sr code will do query for the smartreflex hwmod
> > and will failed, thus avoiding to do further initialization.
> 
> [sp] Trying to avoid big change, and thinking 'narrowly' about this
>      issue in isolation, I had been mulling adding SmartReflex to
>      the omap3_features; and (somehow) using the same.
> 
>      But after noticing the patch related to USBOTG on AM35x, I think
>      original proposal is unambiguous and best way forward.
> 
>      Started working on the patch. Hope to have it ready later tonight
>      or tomorrow.
> 

[sp] Just came across another issue while making this patch:
     Checking for presence of IVA.

     There is not IVA on AM3517. With existing CHIP_IS_3430 flag, the
     hwmod for IVA will be initialized.

     Benoit, Any ideas here?

~sanjeev

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

[Cousson, Benoit]

Hi Sanjeev,

On 2/21/2011 10:57 AM, Premi, Sanjeev wrote:
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
>> Sent: Friday, February 18, 2011 5:43 PM
>> To: Cousson, Benoit
>> Cc: linux-omap@vger.kernel.org
>> Subject: RE: hwmod: multi-omap: disabling smartreflex on AM3517
>>
>>> From: Cousson, Benoit
>>> Sent: Tuesday, February 15, 2011 6:18 PM
>>> To: Premi, Sanjeev
>>> Cc: linux-omap@vger.kernel.org
>>> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
>>>
>>> Hi Sanjeev,
>>>
>>> On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
>>>> AM3517 doesn't support SmartReflex.
>>>>
>>>> However, these HWMODS are defined in omap3xxxx_hwmods[]:
>>>> 	&omap34xx_sr1_hwmod,
>>>> 	&omap34xx_sr2_hwmod,
>>>> 	&omap36xx_sr1_hwmod,
>>>> 	&omap36xx_sr2_hwmod,
>>>>
>>>> (similar definition in l4_slaves as well)
>>>>
>>>> This leads to crash when booting the kernel on AM3517EVM during
>>>> _setup().
>>>>
>>>> I see the field .omap_chip being initialized; but not used.
>>>
>>> Yes, it is. During the hwmod initialization (omap_hwmod_init), only the
>>> hwmods that match the correct chip version are kept.
>>> I guess that your problem is that AM3517 is probably seen as a regular
>>> 3430 for the moment.
>>>
>>>> If I were to use this - along with cpu_is_omap3517(), I would need
>>>> to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
>>>> devices defined in omap_hwmod_3xxx_data.c.
>>>>
>>>> Before going all out on making changes, wanted to check if there is
>>>> a better way. Has this/similar possibility been considered earlier?
>>>
>>> Well, this is the best way to do that for my point of view. This
>>> .omap_chip field was done for that purpose.
>>> During device init, the sr code will do query for the smartreflex hwmod
>>> and will failed, thus avoiding to do further initialization.
>>
>> [sp] Trying to avoid big change, and thinking 'narrowly' about this
>>       issue in isolation, I had been mulling adding SmartReflex to
>>       the omap3_features; and (somehow) using the same.
>>
>>       But after noticing the patch related to USBOTG on AM35x, I think
>>       original proposal is unambiguous and best way forward.
>>
>>       Started working on the patch. Hope to have it ready later tonight
>>       or tomorrow.
>>
>
> [sp] Just came across another issue while making this patch:
>       Checking for presence of IVA.
>
>       There is not IVA on AM3517. With existing CHIP_IS_3430 flag, the
>       hwmod for IVA will be initialized.
>
>       Benoit, Any ideas here?

Yes, still the same one :-).
Since the AM3517 does not contains the exact same list of IPs, you have 
to create a dedicated CHIP_IS_3517 and then change the CHIP_IS_3430 by 
CHIP_IS_3430 | CHIP_IS_3517 to every hwmod entries except SR, IVA and 
any others IP that will not be there.

The hwmod list should be considered as a very details "features" list.
So you should not have to create a new feature list elsewhere. it is a 
duplication of what the hwmod list is already doing. By dumping the 
hwmod list, you should know exactly what is supported by the chip.

I'm quite sure you will have different clock nodes as well, so you will 
have to do the same in the clock_data file.

Regards,
Benoit

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

[Premi, Sanjeev]

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Monday, February 21, 2011 4:01 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> 
> Hi Sanjeev,
> 
> On 2/21/2011 10:57 AM, Premi, Sanjeev wrote:
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> >> Sent: Friday, February 18, 2011 5:43 PM
> >> To: Cousson, Benoit
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: RE: hwmod: multi-omap: disabling smartreflex on AM3517
> >>
> >>> From: Cousson, Benoit
> >>> Sent: Tuesday, February 15, 2011 6:18 PM
> >>> To: Premi, Sanjeev
> >>> Cc: linux-omap@vger.kernel.org
> >>> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> >>>
> >>> Hi Sanjeev,
> >>>
> >>> On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
> >>>> AM3517 doesn't support SmartReflex.
> >>>>
> >>>> However, these HWMODS are defined in omap3xxxx_hwmods[]:
> >>>> 	&omap34xx_sr1_hwmod,
> >>>> 	&omap34xx_sr2_hwmod,
> >>>> 	&omap36xx_sr1_hwmod,
> >>>> 	&omap36xx_sr2_hwmod,
> >>>>
> >>>> (similar definition in l4_slaves as well)
> >>>>
> >>>> This leads to crash when booting the kernel on AM3517EVM during
> >>>> _setup().
> >>>>
> >>>> I see the field .omap_chip being initialized; but not used.
> >>>
> >>> Yes, it is. During the hwmod initialization (omap_hwmod_init), only
> the
> >>> hwmods that match the correct chip version are kept.
> >>> I guess that your problem is that AM3517 is probably seen as a regular
> >>> 3430 for the moment.
> >>>
> >>>> If I were to use this - along with cpu_is_omap3517(), I would need
> >>>> to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
> >>>> devices defined in omap_hwmod_3xxx_data.c.
> >>>>
> >>>> Before going all out on making changes, wanted to check if there is
> >>>> a better way. Has this/similar possibility been considered earlier?
> >>>
> >>> Well, this is the best way to do that for my point of view. This
> >>> .omap_chip field was done for that purpose.
> >>> During device init, the sr code will do query for the smartreflex
> hwmod
> >>> and will failed, thus avoiding to do further initialization.
> >>
> >> [sp] Trying to avoid big change, and thinking 'narrowly' about this
> >>       issue in isolation, I had been mulling adding SmartReflex to
> >>       the omap3_features; and (somehow) using the same.
> >>
> >>       But after noticing the patch related to USBOTG on AM35x, I think
> >>       original proposal is unambiguous and best way forward.
> >>
> >>       Started working on the patch. Hope to have it ready later tonight
> >>       or tomorrow.
> >>
> >
> > [sp] Just came across another issue while making this patch:
> >       Checking for presence of IVA.
> >
> >       There is not IVA on AM3517. With existing CHIP_IS_3430 flag, the
> >       hwmod for IVA will be initialized.
> >
> >       Benoit, Any ideas here?
> 
> Yes, still the same one :-).
> Since the AM3517 does not contains the exact same list of IPs, you have
> to create a dedicated CHIP_IS_3517 and then change the CHIP_IS_3430 by
> CHIP_IS_3430 | CHIP_IS_3517 to every hwmod entries except SR, IVA and
> any others IP that will not be there.
> 
> The hwmod list should be considered as a very details "features" list.
> So you should not have to create a new feature list elsewhere. it is a
> duplication of what the hwmod list is already doing. By dumping the
> hwmod list, you should know exactly what is supported by the chip.
> 
> I'm quite sure you will have different clock nodes as well, so you will
> have to do the same in the clock_data file.

Benoit,

I am only worried about making cpu_is_omap3517() or equiv calls in the
generic functions _init(), _setup() etc.

Gives impression of a hack when I look at the code.

BTW, I would be posting an early version of the patch for review in about
an hour. Have just one more thing to test.

~sanjeev

> 
> Regards,
> Benoit

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

[Cousson, Benoit]

On 2/21/2011 11:39 AM, Premi, Sanjeev wrote:
>> From: Cousson, Benoit
>> Sent: Monday, February 21, 2011 4:01 PM
>>
>> Hi Sanjeev,
>>
>> On 2/21/2011 10:57 AM, Premi, Sanjeev wrote:
>>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>>> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
>>>> Sent: Friday, February 18, 2011 5:43 PM
>>>> To: Cousson, Benoit
>>>> Cc: linux-omap@vger.kernel.org
>>>> Subject: RE: hwmod: multi-omap: disabling smartreflex on AM3517
>>>>
>>>>> From: Cousson, Benoit
>>>>> Sent: Tuesday, February 15, 2011 6:18 PM
>>>>> To: Premi, Sanjeev
>>>>> Cc: linux-omap@vger.kernel.org
>>>>> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
>>>>>
>>>>> Hi Sanjeev,
>>>>>
>>>>> On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
>>>>>> AM3517 doesn't support SmartReflex.
>>>>>>
>>>>>> However, these HWMODS are defined in omap3xxxx_hwmods[]:
>>>>>> 	&omap34xx_sr1_hwmod,
>>>>>> 	&omap34xx_sr2_hwmod,
>>>>>> 	&omap36xx_sr1_hwmod,
>>>>>> 	&omap36xx_sr2_hwmod,
>>>>>>
>>>>>> (similar definition in l4_slaves as well)
>>>>>>
>>>>>> This leads to crash when booting the kernel on AM3517EVM during
>>>>>> _setup().
>>>>>>
>>>>>> I see the field .omap_chip being initialized; but not used.
>>>>>
>>>>> Yes, it is. During the hwmod initialization (omap_hwmod_init), only
>> the
>>>>> hwmods that match the correct chip version are kept.
>>>>> I guess that your problem is that AM3517 is probably seen as a regular
>>>>> 3430 for the moment.
>>>>>
>>>>>> If I were to use this - along with cpu_is_omap3517(), I would need
>>>>>> to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
>>>>>> devices defined in omap_hwmod_3xxx_data.c.
>>>>>>
>>>>>> Before going all out on making changes, wanted to check if there is
>>>>>> a better way. Has this/similar possibility been considered earlier?
>>>>>
>>>>> Well, this is the best way to do that for my point of view. This
>>>>> .omap_chip field was done for that purpose.
>>>>> During device init, the sr code will do query for the smartreflex
>> hwmod
>>>>> and will failed, thus avoiding to do further initialization.
>>>>
>>>> [sp] Trying to avoid big change, and thinking 'narrowly' about this
>>>>        issue in isolation, I had been mulling adding SmartReflex to
>>>>        the omap3_features; and (somehow) using the same.
>>>>
>>>>        But after noticing the patch related to USBOTG on AM35x, I think
>>>>        original proposal is unambiguous and best way forward.
>>>>
>>>>        Started working on the patch. Hope to have it ready later tonight
>>>>        or tomorrow.
>>>>
>>>
>>> [sp] Just came across another issue while making this patch:
>>>        Checking for presence of IVA.
>>>
>>>        There is not IVA on AM3517. With existing CHIP_IS_3430 flag, the
>>>        hwmod for IVA will be initialized.
>>>
>>>        Benoit, Any ideas here?
>>
>> Yes, still the same one :-).
>> Since the AM3517 does not contains the exact same list of IPs, you have
>> to create a dedicated CHIP_IS_3517 and then change the CHIP_IS_3430 by
>> CHIP_IS_3430 | CHIP_IS_3517 to every hwmod entries except SR, IVA and
>> any others IP that will not be there.
>>
>> The hwmod list should be considered as a very details "features" list.
>> So you should not have to create a new feature list elsewhere. it is a
>> duplication of what the hwmod list is already doing. By dumping the
>> hwmod list, you should know exactly what is supported by the chip.
>>
>> I'm quite sure you will have different clock nodes as well, so you will
>> have to do the same in the clock_data file.
> 
> Benoit,
> 
> I am only worried about making cpu_is_omap3517() or equiv calls in the
> generic functions _init(), _setup() etc.
> 
> Gives impression of a hack when I look at the code.

Yes, it is :-)

But you do not have to do that, if you create a CHIP_IS_3517.

It looks like there is some mis-understanding somewhere...


The hwmod filtering is done here based on the omap_chip_is() call.

int __init omap_hwmod_init(struct omap_hwmod **ohs)
{
	struct omap_hwmod *oh;
	int r;

	if (inited)
		return -EINVAL;

	inited = 1;

	if (!ohs)
		return 0;

	oh = *ohs;
	while (oh) {
		if (omap_chip_is(oh->omap_chip)) {
			r = _register(oh);
			WARN(r, "omap_hwmod: %s: _register returned "
			     "%d\n", oh->name, r);
		}
		oh = *++ohs;
	}

	return 0;
}

And the omap_chip is initialized here. So you should add the AM3517 support here (arch/arm/mach-omap2/id.c)

static void __init omap3_check_revision(void)
[...]
	case 0xb868:
		/* Handle OMAP35xx/AM35xx devices
		 *
		 * Set the device to be OMAP3505 here. Actual device
		 * is identified later based on the features.
		 *
		 * REVISIT: AM3505/AM3517 should have their own CHIP_IS
		 */
		omap_revision = OMAP3505_REV(rev);
		omap_chip.oc |= CHIP_IS_OMAP3430ES3_1;
		break;
[...]

The comment is already there BTW, so you just have to replace that by some real code:-)

Regards,
Benoit

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

[Premi, Sanjeev]

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Monday, February 21, 2011 7:48 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org
> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> 
> On 2/21/2011 11:39 AM, Premi, Sanjeev wrote:
> >> From: Cousson, Benoit
> >> Sent: Monday, February 21, 2011 4:01 PM
> >>
> >> Hi Sanjeev,
> >>
> >> On 2/21/2011 10:57 AM, Premi, Sanjeev wrote:
> >>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >>>> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> >>>> Sent: Friday, February 18, 2011 5:43 PM
> >>>> To: Cousson, Benoit
> >>>> Cc: linux-omap@vger.kernel.org
> >>>> Subject: RE: hwmod: multi-omap: disabling smartreflex on AM3517
> >>>>
> >>>>> From: Cousson, Benoit
> >>>>> Sent: Tuesday, February 15, 2011 6:18 PM
> >>>>> To: Premi, Sanjeev
> >>>>> Cc: linux-omap@vger.kernel.org
> >>>>> Subject: Re: hwmod: multi-omap: disabling smartreflex on AM3517
> >>>>>
> >>>>> Hi Sanjeev,
> >>>>>
> >>>>> On 2/15/2011 12:51 PM, Premi, Sanjeev wrote:
> >>>>>> AM3517 doesn't support SmartReflex.
> >>>>>>
> >>>>>> However, these HWMODS are defined in omap3xxxx_hwmods[]:
> >>>>>> 	&omap34xx_sr1_hwmod,
> >>>>>> 	&omap34xx_sr2_hwmod,
> >>>>>> 	&omap36xx_sr1_hwmod,
> >>>>>> 	&omap36xx_sr2_hwmod,
> >>>>>>
> >>>>>> (similar definition in l4_slaves as well)
> >>>>>>
> >>>>>> This leads to crash when booting the kernel on AM3517EVM during
> >>>>>> _setup().
> >>>>>>
> >>>>>> I see the field .omap_chip being initialized; but not used.
> >>>>>
> >>>>> Yes, it is. During the hwmod initialization (omap_hwmod_init), only
> >> the
> >>>>> hwmods that match the correct chip version are kept.
> >>>>> I guess that your problem is that AM3517 is probably seen as a
> regular
> >>>>> 3430 for the moment.
> >>>>>
> >>>>>> If I were to use this - along with cpu_is_omap3517(), I would need
> >>>>>> to define a new flag e.g. CHIP_IS_AM3517 and add it to almost all
> >>>>>> devices defined in omap_hwmod_3xxx_data.c.
> >>>>>>
> >>>>>> Before going all out on making changes, wanted to check if there is
> >>>>>> a better way. Has this/similar possibility been considered earlier?
> >>>>>
> >>>>> Well, this is the best way to do that for my point of view. This
> >>>>> .omap_chip field was done for that purpose.
> >>>>> During device init, the sr code will do query for the smartreflex
> >> hwmod
> >>>>> and will failed, thus avoiding to do further initialization.
> >>>>
> >>>> [sp] Trying to avoid big change, and thinking 'narrowly' about this
> >>>>        issue in isolation, I had been mulling adding SmartReflex to
> >>>>        the omap3_features; and (somehow) using the same.
> >>>>
> >>>>        But after noticing the patch related to USBOTG on AM35x, I
> think
> >>>>        original proposal is unambiguous and best way forward.
> >>>>
> >>>>        Started working on the patch. Hope to have it ready later
> tonight
> >>>>        or tomorrow.
> >>>>
> >>>
> >>> [sp] Just came across another issue while making this patch:
> >>>        Checking for presence of IVA.
> >>>
> >>>        There is not IVA on AM3517. With existing CHIP_IS_3430 flag,
> the
> >>>        hwmod for IVA will be initialized.
> >>>
> >>>        Benoit, Any ideas here?
> >>
> >> Yes, still the same one :-).
> >> Since the AM3517 does not contains the exact same list of IPs, you have
> >> to create a dedicated CHIP_IS_3517 and then change the CHIP_IS_3430 by
> >> CHIP_IS_3430 | CHIP_IS_3517 to every hwmod entries except SR, IVA and
> >> any others IP that will not be there.
> >>
> >> The hwmod list should be considered as a very details "features" list.
> >> So you should not have to create a new feature list elsewhere. it is a
> >> duplication of what the hwmod list is already doing. By dumping the
> >> hwmod list, you should know exactly what is supported by the chip.
> >>
> >> I'm quite sure you will have different clock nodes as well, so you will
> >> have to do the same in the clock_data file.
> >
> > Benoit,
> >
> > I am only worried about making cpu_is_omap3517() or equiv calls in the
> > generic functions _init(), _setup() etc.
> >
> > Gives impression of a hack when I look at the code.
> 
> Yes, it is :-)
> 
> But you do not have to do that, if you create a CHIP_IS_3517.
> 
> It looks like there is some mis-understanding somewhere...
> 
> 
> The hwmod filtering is done here based on the omap_chip_is() call.
> 
> int __init omap_hwmod_init(struct omap_hwmod **ohs)
> {
> 	struct omap_hwmod *oh;
> 	int r;
> 
> 	if (inited)
> 		return -EINVAL;
> 
> 	inited = 1;
> 
> 	if (!ohs)
> 		return 0;
> 
> 	oh = *ohs;
> 	while (oh) {
> 		if (omap_chip_is(oh->omap_chip)) {
> 			r = _register(oh);
> 			WARN(r, "omap_hwmod: %s: _register returned "
> 			     "%d\n", oh->name, r);
> 		}
> 		oh = *++ohs;
> 	}
> 
> 	return 0;
> }
> 
> And the omap_chip is initialized here. So you should add the AM3517
> support here (arch/arm/mach-omap2/id.c)
> 
> static void __init omap3_check_revision(void)
> [...]
> 	case 0xb868:
> 		/* Handle OMAP35xx/AM35xx devices
> 		 *
> 		 * Set the device to be OMAP3505 here. Actual device
> 		 * is identified later based on the features.
> 		 *
> 		 * REVISIT: AM3505/AM3517 should have their own CHIP_IS
> 		 */
> 		omap_revision = OMAP3505_REV(rev);
> 		omap_chip.oc |= CHIP_IS_OMAP3430ES3_1;
> 		break;
> [...]
> 
> 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.

     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?

~sanjeev

> 
> Regards,
> Benoit

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

[Cousson, Benoit]

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.

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.

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.

I think I still prefer the first solution.

Regards,
Benoit

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

[Premi, Sanjeev]

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.

~sanjeev

>
> I think I still prefer the first solution.
>
> Regards,
> Benoit
>
>

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

[Cousson, Benoit]

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

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

[Premi, Sanjeev]

>From: Cousson, Benoit
>Sent: Wednesday, February 23, 2011 4:23 AM
>To: Premi, Sanjeev
>Cc: linux-omap@vger.kernel.org; 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
>

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

[Cousson, Benoit]

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@vger.kernel.org; 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?

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

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

[Premi, Sanjeev]

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Wednesday, February 23, 2011 9:23 PM
> To: Premi, Sanjeev
> Cc: linux-omap@vger.kernel.org; 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@vger.kernel.org; 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

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

[Cousson, Benoit]

Hi Sanjeev,

On 2/25/2011 12:39 PM, Premi, Sanjeev wrote:
>> From: Cousson, Benoit
>> Sent: Wednesday, February 23, 2011 9:23 PM

[...]

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

I didn't anticipated that either :-(

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

I'm afraid, we will probably end up with a dynamic binding solution like 
what we have to do for the clock framework for the similar reason.
We can optimize the size but it will impact the performance... the usual 
trade off.

Thanks,
Benoit