RE: [PATCH 05/13] OMAP: Introduce device specific set rate and get rate in device opp structures.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>>-----Original Message-----
>>From: Cousson, Benoit
>>Sent: Saturday, September 18, 2010 3:43 PM
>>To: Gopinath, Thara
>>Cc: Kevin Hilman; Paul Walmsley; linux-omap@xxxxxxxxxxxxxxx; Sripathy,
>>Vishwanath; Sawant, Anand
>>Subject: Re: [PATCH 05/13] OMAP: Introduce device specific set rate and get
>>rate in device opp structures.
>>
>>Hi Thara,
>>
>>On 9/17/2010 4:55 PM, Gopinath, Thara wrote:
>>>
>>>>> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx]
>>>>> Sent: Thursday, September 16, 2010 8:58 PM
>>>>>
>>>>> "Gopinath, Thara"<thara@xxxxxx>  writes:
>>>>>
>>>>>>>> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx]
>>>>>>>> Sent: Friday, September 03, 2010 5:12 AM
>>>>>>>>
>>>>>>>> Thara Gopinath<thara@xxxxxx>  writes:
>>>>>>>>
>>>>>>>>> This patch extends the device opp structure to contain
>>>>>>>>> pointers to scale the operating rate of the
>>>>>>>>> device and to retrieve the operating rate of the device.
>>>>>>>>> This patch also adds the three new APIs in the opp layer
>>>>>>>>> namely opp_set_rate that can be called to set a new operating
>>>>>>>>> rate for a device, opp_get_rate that can be called to retrieve
>>>>>>>>> the operating frequency for a device and opp_populate_rate_fns
>>>>>>>>> to populte the device specific set_rate and get_rate API's.
>>>>>>>>> The opp_set_rate and opp_get_rate does some routine error
>>>>>>>>> checks and finally calls into the device specific set_rate
>>>>>>>>> and get_rate APIs populated through opp_populate_rate_fns.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Thara Gopinath<thara@xxxxxx>
>>>>>>>>
>>>>>>>> As I think about this more, I'm not sure the OPP layer is the right
>>>>>>>> layer for the get/set rate functions.  The OPP layer is currently just
>>>>>>>> the data store for OPP data. To me, these set/get functions are better
>>>>>>>> associated directly with an omap_device instead of an OPP.
>>>>>>>>
>>>>>>>> For instance, the new OPP APIs are a bit confusing in terms of OPPs.
>>>>>>>> e.g: opp_get_rate().  What is the "rate" of an OPP, and what's the
>>>>>>>> difference with opp_get_freq()?
>>>>>>>>
>>>>>>>> What's really happening is the rate is being changed for a device, and
>>>>>>>> the need for specific hooks are at the device level, not the OPP
>>level.
>>>>>>>> For example, this current approach would not scale if you needed
>>>>>>>> multiple devices in the same domain that each needed custom
>>>>>>>> set_rate/get_rate hooks.
>>>>>>>>
>>>>>>>> So instead, what about adding custom hooks at the omap_device level?
>>>>>>>> They could be registered at omap_device_build() time in the device
>>>>>>>> specific code.
>>>>>>
>>>>>> This is exactly what I had in my mind when I started implementing this.
>>>>>> But then Paul said hwmod or omap_device is not the place to keep it.
>>>>>
>>>>> IIRC, Paul's concern was that *hwmod* was not the right place for this
>>>>> (and I agree.)  However, I think omap_device is the right place for
>>>>> this.
>>>>>
>>>>> Paul?
>>>>>
>>>>>> Also I do not want the set rate get rate APIs for devices that require
>>>>>> changes dividers in the PRCM clock module to be spread out in the
>>>>>> system. Makes it very very difficult to debug.  If we agree to add the
>>>>>> set_rate and get_rate in the omap_device structure, I would like to
>>>>>> have one more API in the omap_device layer to register these APIs
>>>>>> device wise. Thus we can keep these set rate and get rate APIs in one
>>>>>> single file.
>>>>>
>>>>> Agreed.  And those functions should be in the respective
>>>>> device-specific file, where the omap_device_build() etc are done for
>>>>> that device.
>>>
>>> Hello Kevin,
>>>
>>> I basically agree with all your other suggestions except this. I do not
>>> think device files are the correct place for this. For starters definitely
>>mpu,
>>> l3 and iva devices are not built from any device file. I do not like the
>>idea
>>> of device set rate APIs spread across different files.
>>
>>I don't not understand the reason?
>>If we need to add specific device function for mpu, l3, or iva, we can
>>easily add a file to contain that.
>>BTW, thanks to work done by Santosh and Felipe we will probably soon
>>introduce a l3 driver that will allow to handle interconnect errors. We
>>will thus have a real device for l3 as well.
>>
>>A device set_rate is clearly device specific. If a device will have to
>>play with its own internal dividers along with PRCM dividers, that code
>>still belong to the device.
>>
>>I do not see the need nor the advantage to centralize that? You will
>>end-up with a huge file that every driver owners will have to hack in
>>order to add set_rate support for their device.
>>
>>At device build time, IP with set_rate support will just add non-null
>>parameters to the omap_device_build(), et voila.
>>
>>For the debug point of view, you can just add a new sysfs entry for
>>scalable devices that will allow you to track scalable device vs regular
>>ones.

Hello Benoit/Kevin,

Most of the devices need not scale any internal dividers. For devices that 
need to scale internal dividers, I agree that the set_rate and get_rate should come from the devices file. But for other devices that involve only PRCM dividers, I do not think they should be bothered with implementing these APIs and maintaining them.

Regards
Thara
--
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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux