On 11/26/2012 10:02 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Nov 26, 2012 at 05:14:45PM +0200, Roger Quadros wrote:
>> Felipe,
>>
>> On 11/21/2012 03:39 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Thu, Nov 15, 2012 at 04:34:04PM +0200, Roger Quadros wrote:
>>>> All ports have similarly named port clocks so we can
>>>> bunch them into a port data structure and use for loop
>>>> to enable/disable the clocks.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>>>> ---
>>>> drivers/mfd/omap-usb-host.c | 208 +++++++++++++++++++++----------------------
>>>> 1 files changed, 101 insertions(+), 107 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>>>> index 23cec57..7303c41 100644
>>>> --- a/drivers/mfd/omap-usb-host.c
>>>> +++ b/drivers/mfd/omap-usb-host.c
>>>> @@ -76,6 +76,8 @@
>>>>
>>>> #define OMAP_UHH_DEBUG_CSR (0x44)
>>>>
>>>> +#define MAX_HS_USB_PORTS 3 /* Increase this if any chip has more */
>>>> +
>>>> /* Values of UHH_REVISION - Note: these are not given in the TRM */
>>>> #define OMAP_USBHS_REV1 0x00000010 /* OMAP3 */
>>>> #define OMAP_USBHS_REV2 0x50700100 /* OMAP4 */
>>>> @@ -87,14 +89,15 @@
>>>> #define is_ehci_tll_mode(x) (x == OMAP_EHCI_PORT_MODE_TLL)
>>>> #define is_ehci_hsic_mode(x) (x == OMAP_EHCI_PORT_MODE_HSIC)
>>>>
>>>> +struct usbhs_port {
>>>> + struct clk *utmi_clk;
>>>> +};
>>>
>>> I rather not since this will make it a lot more difficult to use
>>> pm_clk_add() :-s Also, this sort of thing should be dynamically
>>> allocated anyway ;-)
>>>
>>
>> Why do you say so? The whole point of this patch is to group similarly
>> named clocks so that we can use a for loop and set number of ports (or
>> clocks) dynamically. I suppose it would be just a matter of replacing
>> clk_enable/disable() with pm_clk_add() later, right?
>>
>> If you see patch 11, we are adding 2 HSIC related clocks to this
>> structure. This means 9 clocks (i.e. 3 clocks for 3 ports) can be
>> managed using a simple for loop instead of coding each clock name by hand.
>
> that's usually not what you want, actually. You want clock management to
> be explicit so you can have micro-power optimizations. I fear that the
> time omap-usb-host.c gets *truly* stabilized and we move to more
> aggressive PM, we will undo these changes just to have a more granular
> control of the clocks, at which point your patch would be rendered
> useless.
>
The granularity is still there, just that port clocks are grouped
together. Do you think it is better if I get rid of 'struct usbhs_port'
and keep the clocks in the main structure instead?
e.g.
struct usbhs_hcd_omap {
struct clk **utmi_clk;
struct clk **hsic1_clk;
struct clk **hsic2_clk;
...
}
The clocks can then be accessed as follows
omap->utmi_clk[i]; /* i is port number */
Does this sound OK to you?
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
