diff mbox

[U-Boot,v3,5/7] usb: host: ohci-generic: add CLOCK support

Message ID 1495028048-6310-6-git-send-email-patrice.chotard@st.com
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Patrice CHOTARD May 17, 2017, 1:34 p.m. UTC
From: Patrice Chotard <patrice.chotard@st.com>

use list to save reference to enabled clocks in order to
disabled them in case of error during probe() or
during driver removal.

Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---

v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
	_ keep enabled clocks reference in list in order to 
	  disable clocks in error path or in .remove callback

v2:	_ add error path management
	_ add .remove callback

 drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

Comments

Marek Vasut May 18, 2017, 9:29 a.m. UTC | #1
On 05/17/2017 03:34 PM, patrice.chotard@st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> use list to save reference to enabled clocks in order to
> disabled them in case of error during probe() or
> during driver removal.
> 
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> 
> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
> 	_ keep enabled clocks reference in list in order to 
> 	  disable clocks in error path or in .remove callback
> 
> v2:	_ add error path management
> 	_ add .remove callback
> 
>  drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index f3307f4..a6d89a8 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>  #include "ohci.h"
>  
> @@ -12,20 +13,98 @@
>  # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>  #endif
>  
> +struct ohci_clock {
> +	struct clk *clk;
> +	struct list_head list;
> +};
> +
>  struct generic_ohci {
>  	ohci_t ohci;
> +	struct list_head clks;
>  };
>  
> +static int ohci_release_clocks(struct generic_ohci *priv)
> +{
> +	struct ohci_clock *ohci_clock, *tmp;
> +	struct clk *clk;
> +	int ret;
> +
> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
> +		clk = ohci_clock->clk;
> +
> +		clk_request(clk->dev, clk);
> +		if (ret)
> +			return ret;
> +
> +		clk_disable(clk);
> +
> +		ret = clk_free(clk);
> +		if (ret)
> +			return ret;
> +
> +		list_del(&ohci_clock->list);
> +	}
> +	return 0;
> +}
> +
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>  	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
> +	struct generic_ohci *priv = dev_get_priv(dev);
> +	int i, ret;
> +
> +	INIT_LIST_HEAD(&priv->clks);
> +
> +	for (i = 0; ; i++) {
> +		struct ohci_clock *ohci_clock;
> +		struct clk *clk;
> +
> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);

Since you know how many entries the clock phandle has, you can allocate
an array and drop this while list handling and this per-element kmalloc,
which fragments the allocator pool.

> +		if (!clk) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +
> +		ret = clk_get_by_index(dev, i, clk);
> +		if (ret < 0)
> +			break;
> +
> +		if (clk_enable(clk)) {
> +			error("failed to enable ohci_clock %d\n", i);
> +			clk_free(clk);
> +			goto clk_err;
> +		}
> +		clk_free(clk);
> +
> +		/*
> +		 * add enabled clocks into clks list in order to be disabled
> +		 * later on ohci_usb_remove() call or in error path if needed
> +		 */
> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);

Can't you just embed one structure into the other ?

> +		if (!ohci_clock) {
> +			error("Can't allocate resource\n");
> +			goto clk_err;
> +		}
> +		ohci_clock->clk = clk;
> +		list_add(&ohci_clock->list, &priv->clks);
> +	}
>  
>  	return ohci_register(dev, regs);
> +
> +clk_err:
> +	return ohci_release_clocks(priv);
>  }
>  
>  static int ohci_usb_remove(struct udevice *dev)
>  {
> -	return ohci_deregister(dev);
> +	struct generic_ohci *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = ohci_deregister(dev);
> +	if (ret)
> +		return ret;
> +
> +	return ohci_release_clocks(priv);
>  }
>  
>  static const struct udevice_id ohci_usb_ids[] = {
>
Patrice CHOTARD May 19, 2017, 11:27 a.m. UTC | #2
Hi Marek

On 05/18/2017 11:29 AM, Marek Vasut wrote:
> On 05/17/2017 03:34 PM, patrice.chotard@st.com wrote:

>> From: Patrice Chotard <patrice.chotard@st.com>

>>

>> use list to save reference to enabled clocks in order to

>> disabled them in case of error during probe() or

>> during driver removal.

>>

>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>

>> ---

>>

>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5

>> 	_ keep enabled clocks reference in list in order to

>> 	  disable clocks in error path or in .remove callback

>>

>> v2:	_ add error path management

>> 	_ add .remove callback

>>

>>   drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-

>>   1 file changed, 80 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c

>> index f3307f4..a6d89a8 100644

>> --- a/drivers/usb/host/ohci-generic.c

>> +++ b/drivers/usb/host/ohci-generic.c

>> @@ -5,6 +5,7 @@

>>    */

>>   

>>   #include <common.h>

>> +#include <clk.h>

>>   #include <dm.h>

>>   #include "ohci.h"

>>   

>> @@ -12,20 +13,98 @@

>>   # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"

>>   #endif

>>   

>> +struct ohci_clock {

>> +	struct clk *clk;

>> +	struct list_head list;

>> +};

>> +

>>   struct generic_ohci {

>>   	ohci_t ohci;

>> +	struct list_head clks;

>>   };

>>   

>> +static int ohci_release_clocks(struct generic_ohci *priv)

>> +{

>> +	struct ohci_clock *ohci_clock, *tmp;

>> +	struct clk *clk;

>> +	int ret;

>> +

>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {

>> +		clk = ohci_clock->clk;

>> +

>> +		clk_request(clk->dev, clk);

>> +		if (ret)

>> +			return ret;

>> +

>> +		clk_disable(clk);

>> +

>> +		ret = clk_free(clk);

>> +		if (ret)

>> +			return ret;

>> +

>> +		list_del(&ohci_clock->list);

>> +	}

>> +	return 0;

>> +}

>> +

>>   static int ohci_usb_probe(struct udevice *dev)

>>   {

>>   	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);

>> +	struct generic_ohci *priv = dev_get_priv(dev);

>> +	int i, ret;

>> +

>> +	INIT_LIST_HEAD(&priv->clks);

>> +

>> +	for (i = 0; ; i++) {

>> +		struct ohci_clock *ohci_clock;

>> +		struct clk *clk;

>> +

>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);

> 

> Since you know how many entries the clock phandle has, you can allocate

> an array and drop this while list handling and this per-element kmalloc,

> which fragments the allocator pool.


I disagree, at this point we don't know how many entries the clock 
phandle has.

But, following your idea to avoid allocator pool fragmentation, in order 
to get the phandle number for array allocation, i can, for example add a 
new fdt API :

int fdtdec_get_phandle_nb(const void *blob, int src_node,
				   const char *list_name,
				   const char *cells_name,
				   int cell_count,
				   int phandle_nb);

Then, with phandle_nb,, we 'll be able to allocate the right array size 
for clocks and resets before populating them.

Thanks

Patrice

> 

>> +		if (!clk) {

>> +			error("Can't allocate resource\n");

>> +			goto clk_err;

>> +		}

>> +

>> +		ret = clk_get_by_index(dev, i, clk);

>> +		if (ret < 0)

>> +			break;

>> +

>> +		if (clk_enable(clk)) {

>> +			error("failed to enable ohci_clock %d\n", i);

>> +			clk_free(clk);

>> +			goto clk_err;

>> +		}

>> +		clk_free(clk);

>> +

>> +		/*

>> +		 * add enabled clocks into clks list in order to be disabled

>> +		 * later on ohci_usb_remove() call or in error path if needed

>> +		 */

>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);

> 

> Can't you just embed one structure into the other ?

> 

>> +		if (!ohci_clock) {

>> +			error("Can't allocate resource\n");

>> +			goto clk_err;

>> +		}

>> +		ohci_clock->clk = clk;

>> +		list_add(&ohci_clock->list, &priv->clks);

>> +	}

>>   

>>   	return ohci_register(dev, regs);

>> +

>> +clk_err:

>> +	return ohci_release_clocks(priv);

>>   }

>>   

>>   static int ohci_usb_remove(struct udevice *dev)

>>   {

>> -	return ohci_deregister(dev);

>> +	struct generic_ohci *priv = dev_get_priv(dev);

>> +	int ret;

>> +

>> +	ret = ohci_deregister(dev);

>> +	if (ret)

>> +		return ret;

>> +

>> +	return ohci_release_clocks(priv);

>>   }

>>   

>>   static const struct udevice_id ohci_usb_ids[] = {

>>

> 

>
Marek Vasut May 19, 2017, 11:51 a.m. UTC | #3
On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:
> Hi Marek
> 
> On 05/18/2017 11:29 AM, Marek Vasut wrote:
>> On 05/17/2017 03:34 PM, patrice.chotard@st.com wrote:
>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>
>>> use list to save reference to enabled clocks in order to
>>> disabled them in case of error during probe() or
>>> during driver removal.
>>>
>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>> ---
>>>
>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>>> 	_ keep enabled clocks reference in list in order to
>>> 	  disable clocks in error path or in .remove callback
>>>
>>> v2:	_ add error path management
>>> 	_ add .remove callback
>>>
>>>   drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>>> index f3307f4..a6d89a8 100644
>>> --- a/drivers/usb/host/ohci-generic.c
>>> +++ b/drivers/usb/host/ohci-generic.c
>>> @@ -5,6 +5,7 @@
>>>    */
>>>   
>>>   #include <common.h>
>>> +#include <clk.h>
>>>   #include <dm.h>
>>>   #include "ohci.h"
>>>   
>>> @@ -12,20 +13,98 @@
>>>   # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>>   #endif
>>>   
>>> +struct ohci_clock {
>>> +	struct clk *clk;
>>> +	struct list_head list;
>>> +};
>>> +
>>>   struct generic_ohci {
>>>   	ohci_t ohci;
>>> +	struct list_head clks;
>>>   };
>>>   
>>> +static int ohci_release_clocks(struct generic_ohci *priv)
>>> +{
>>> +	struct ohci_clock *ohci_clock, *tmp;
>>> +	struct clk *clk;
>>> +	int ret;
>>> +
>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>>> +		clk = ohci_clock->clk;
>>> +
>>> +		clk_request(clk->dev, clk);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		clk_disable(clk);
>>> +
>>> +		ret = clk_free(clk);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		list_del(&ohci_clock->list);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>   static int ohci_usb_probe(struct udevice *dev)
>>>   {
>>>   	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
>>> +	struct generic_ohci *priv = dev_get_priv(dev);
>>> +	int i, ret;
>>> +
>>> +	INIT_LIST_HEAD(&priv->clks);
>>> +
>>> +	for (i = 0; ; i++) {
>>> +		struct ohci_clock *ohci_clock;
>>> +		struct clk *clk;
>>> +
>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
>>
>> Since you know how many entries the clock phandle has, you can allocate
>> an array and drop this while list handling and this per-element kmalloc,
>> which fragments the allocator pool.
> 
> I disagree, at this point we don't know how many entries the clock 
> phandle has.

I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,
which is already better.

> But, following your idea to avoid allocator pool fragmentation, in order 
> to get the phandle number for array allocation, i can, for example add a 
> new fdt API :
> 
> int fdtdec_get_phandle_nb(const void *blob, int src_node,
> 				   const char *list_name,
> 				   const char *cells_name,
> 				   int cell_count,
> 				   int phandle_nb);

Uh, why ?

> Then, with phandle_nb,, we 'll be able to allocate the right array size 
> for clocks and resets before populating them.

Just query the number of elements up front and then allocate the array ?

> Thanks
> 
> Patrice
> 
>>
>>> +		if (!clk) {
>>> +			error("Can't allocate resource\n");
>>> +			goto clk_err;
>>> +		}
>>> +
>>> +		ret = clk_get_by_index(dev, i, clk);
>>> +		if (ret < 0)
>>> +			break;
>>> +
>>> +		if (clk_enable(clk)) {
>>> +			error("failed to enable ohci_clock %d\n", i);
>>> +			clk_free(clk);
>>> +			goto clk_err;
>>> +		}
>>> +		clk_free(clk);
>>> +
>>> +		/*
>>> +		 * add enabled clocks into clks list in order to be disabled
>>> +		 * later on ohci_usb_remove() call or in error path if needed
>>> +		 */
>>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
>>
>> Can't you just embed one structure into the other ?
>>
>>> +		if (!ohci_clock) {
>>> +			error("Can't allocate resource\n");
>>> +			goto clk_err;
>>> +		}
>>> +		ohci_clock->clk = clk;
>>> +		list_add(&ohci_clock->list, &priv->clks);
>>> +	}
>>>   
>>>   	return ohci_register(dev, regs);
>>> +
>>> +clk_err:
>>> +	return ohci_release_clocks(priv);
>>>   }
>>>   
>>>   static int ohci_usb_remove(struct udevice *dev)
>>>   {
>>> -	return ohci_deregister(dev);
>>> +	struct generic_ohci *priv = dev_get_priv(dev);
>>> +	int ret;
>>> +
>>> +	ret = ohci_deregister(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return ohci_release_clocks(priv);
>>>   }
>>>   
>>>   static const struct udevice_id ohci_usb_ids[] = {
>>>
>>
Patrice CHOTARD May 19, 2017, 12:16 p.m. UTC | #4
On 05/19/2017 01:51 PM, Marek Vasut wrote:
> On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:

>> Hi Marek

>>

>> On 05/18/2017 11:29 AM, Marek Vasut wrote:

>>> On 05/17/2017 03:34 PM, patrice.chotard@st.com wrote:

>>>> From: Patrice Chotard <patrice.chotard@st.com>

>>>>

>>>> use list to save reference to enabled clocks in order to

>>>> disabled them in case of error during probe() or

>>>> during driver removal.

>>>>

>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>

>>>> ---

>>>>

>>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5

>>>> 	_ keep enabled clocks reference in list in order to

>>>> 	  disable clocks in error path or in .remove callback

>>>>

>>>> v2:	_ add error path management

>>>> 	_ add .remove callback

>>>>

>>>>    drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-

>>>>    1 file changed, 80 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c

>>>> index f3307f4..a6d89a8 100644

>>>> --- a/drivers/usb/host/ohci-generic.c

>>>> +++ b/drivers/usb/host/ohci-generic.c

>>>> @@ -5,6 +5,7 @@

>>>>     */

>>>>    

>>>>    #include <common.h>

>>>> +#include <clk.h>

>>>>    #include <dm.h>

>>>>    #include "ohci.h"

>>>>    

>>>> @@ -12,20 +13,98 @@

>>>>    # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"

>>>>    #endif

>>>>    

>>>> +struct ohci_clock {

>>>> +	struct clk *clk;

>>>> +	struct list_head list;

>>>> +};

>>>> +

>>>>    struct generic_ohci {

>>>>    	ohci_t ohci;

>>>> +	struct list_head clks;

>>>>    };

>>>>    

>>>> +static int ohci_release_clocks(struct generic_ohci *priv)

>>>> +{

>>>> +	struct ohci_clock *ohci_clock, *tmp;

>>>> +	struct clk *clk;

>>>> +	int ret;

>>>> +

>>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {

>>>> +		clk = ohci_clock->clk;

>>>> +

>>>> +		clk_request(clk->dev, clk);

>>>> +		if (ret)

>>>> +			return ret;

>>>> +

>>>> +		clk_disable(clk);

>>>> +

>>>> +		ret = clk_free(clk);

>>>> +		if (ret)

>>>> +			return ret;

>>>> +

>>>> +		list_del(&ohci_clock->list);

>>>> +	}

>>>> +	return 0;

>>>> +}

>>>> +

>>>>    static int ohci_usb_probe(struct udevice *dev)

>>>>    {

>>>>    	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);

>>>> +	struct generic_ohci *priv = dev_get_priv(dev);

>>>> +	int i, ret;

>>>> +

>>>> +	INIT_LIST_HEAD(&priv->clks);

>>>> +

>>>> +	for (i = 0; ; i++) {

>>>> +		struct ohci_clock *ohci_clock;

>>>> +		struct clk *clk;

>>>> +

>>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);

>>>

>>> Since you know how many entries the clock phandle has, you can allocate

>>> an array and drop this while list handling and this per-element kmalloc,

>>> which fragments the allocator pool.

>>

>> I disagree, at this point we don't know how many entries the clock

>> phandle has.

> 

> I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,

> which is already better.


Can you elaborate ?

> 

>> But, following your idea to avoid allocator pool fragmentation, in order

>> to get the phandle number for array allocation, i can, for example add a

>> new fdt API :

>>

>> int fdtdec_get_phandle_nb(const void *blob, int src_node,

>> 				   const char *list_name,

>> 				   const char *cells_name,

>> 				   int cell_count,

>> 				   int phandle_nb);

> 

> Uh, why ?

> 

>> Then, with phandle_nb,, we 'll be able to allocate the right array size

>> for clocks and resets before populating them.

> 

> Just query the number of elements up front and then allocate the array ?


Can you indicate me with which API please ?

> 

>> Thanks

>>

>> Patrice

>>

>>>

>>>> +		if (!clk) {

>>>> +			error("Can't allocate resource\n");

>>>> +			goto clk_err;

>>>> +		}

>>>> +

>>>> +		ret = clk_get_by_index(dev, i, clk);

>>>> +		if (ret < 0)

>>>> +			break;

>>>> +

>>>> +		if (clk_enable(clk)) {

>>>> +			error("failed to enable ohci_clock %d\n", i);

>>>> +			clk_free(clk);

>>>> +			goto clk_err;

>>>> +		}

>>>> +		clk_free(clk);

>>>> +

>>>> +		/*

>>>> +		 * add enabled clocks into clks list in order to be disabled

>>>> +		 * later on ohci_usb_remove() call or in error path if needed

>>>> +		 */

>>>> +		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);

>>>

>>> Can't you just embed one structure into the other ?

>>>

>>>> +		if (!ohci_clock) {

>>>> +			error("Can't allocate resource\n");

>>>> +			goto clk_err;

>>>> +		}

>>>> +		ohci_clock->clk = clk;

>>>> +		list_add(&ohci_clock->list, &priv->clks);

>>>> +	}

>>>>    

>>>>    	return ohci_register(dev, regs);

>>>> +

>>>> +clk_err:

>>>> +	return ohci_release_clocks(priv);

>>>>    }

>>>>    

>>>>    static int ohci_usb_remove(struct udevice *dev)

>>>>    {

>>>> -	return ohci_deregister(dev);

>>>> +	struct generic_ohci *priv = dev_get_priv(dev);

>>>> +	int ret;

>>>> +

>>>> +	ret = ohci_deregister(dev);

>>>> +	if (ret)

>>>> +		return ret;

>>>> +

>>>> +	return ohci_release_clocks(priv);

>>>>    }

>>>>    

>>>>    static const struct udevice_id ohci_usb_ids[] = {

>>>>

>>>

> 

>
Marek Vasut May 20, 2017, 4:16 p.m. UTC | #5
On 05/19/2017 02:16 PM, Patrice CHOTARD wrote:
> On 05/19/2017 01:51 PM, Marek Vasut wrote:
>> On 05/19/2017 01:27 PM, Patrice CHOTARD wrote:
>>> Hi Marek
>>>
>>> On 05/18/2017 11:29 AM, Marek Vasut wrote:
>>>> On 05/17/2017 03:34 PM, patrice.chotard@st.com wrote:
>>>>> From: Patrice Chotard <patrice.chotard@st.com>
>>>>>
>>>>> use list to save reference to enabled clocks in order to
>>>>> disabled them in case of error during probe() or
>>>>> during driver removal.
>>>>>
>>>>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>>>>> ---
>>>>>
>>>>> v3:	_ extract in this patch the CLOCK support add-on from previous patch 5
>>>>> 	_ keep enabled clocks reference in list in order to
>>>>> 	  disable clocks in error path or in .remove callback
>>>>>
>>>>> v2:	_ add error path management
>>>>> 	_ add .remove callback
>>>>>
>>>>>    drivers/usb/host/ohci-generic.c | 81 ++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 80 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
>>>>> index f3307f4..a6d89a8 100644
>>>>> --- a/drivers/usb/host/ohci-generic.c
>>>>> +++ b/drivers/usb/host/ohci-generic.c
>>>>> @@ -5,6 +5,7 @@
>>>>>     */
>>>>>    
>>>>>    #include <common.h>
>>>>> +#include <clk.h>
>>>>>    #include <dm.h>
>>>>>    #include "ohci.h"
>>>>>    
>>>>> @@ -12,20 +13,98 @@
>>>>>    # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
>>>>>    #endif
>>>>>    
>>>>> +struct ohci_clock {
>>>>> +	struct clk *clk;
>>>>> +	struct list_head list;
>>>>> +};
>>>>> +
>>>>>    struct generic_ohci {
>>>>>    	ohci_t ohci;
>>>>> +	struct list_head clks;
>>>>>    };
>>>>>    
>>>>> +static int ohci_release_clocks(struct generic_ohci *priv)
>>>>> +{
>>>>> +	struct ohci_clock *ohci_clock, *tmp;
>>>>> +	struct clk *clk;
>>>>> +	int ret;
>>>>> +
>>>>> +	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
>>>>> +		clk = ohci_clock->clk;
>>>>> +
>>>>> +		clk_request(clk->dev, clk);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		clk_disable(clk);
>>>>> +
>>>>> +		ret = clk_free(clk);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		list_del(&ohci_clock->list);
>>>>> +	}
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>    static int ohci_usb_probe(struct udevice *dev)
>>>>>    {
>>>>>    	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
>>>>> +	struct generic_ohci *priv = dev_get_priv(dev);
>>>>> +	int i, ret;
>>>>> +
>>>>> +	INIT_LIST_HEAD(&priv->clks);
>>>>> +
>>>>> +	for (i = 0; ; i++) {
>>>>> +		struct ohci_clock *ohci_clock;
>>>>> +		struct clk *clk;
>>>>> +
>>>>> +		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
>>>>
>>>> Since you know how many entries the clock phandle has, you can allocate
>>>> an array and drop this while list handling and this per-element kmalloc,
>>>> which fragments the allocator pool.
>>>
>>> I disagree, at this point we don't know how many entries the clock
>>> phandle has.
>>
>> I know you can do it in less then 2 mallocs,  in fact in exactly 1 ,
>> which is already better.
> 
> Can you elaborate ?
> 
>>
>>> But, following your idea to avoid allocator pool fragmentation, in order
>>> to get the phandle number for array allocation, i can, for example add a
>>> new fdt API :
>>>
>>> int fdtdec_get_phandle_nb(const void *blob, int src_node,
>>> 				   const char *list_name,
>>> 				   const char *cells_name,
>>> 				   int cell_count,
>>> 				   int phandle_nb);
>>
>> Uh, why ?
>>
>>> Then, with phandle_nb,, we 'll be able to allocate the right array size
>>> for clocks and resets before populating them.
>>
>> Just query the number of elements up front and then allocate the array ?
> 
> Can you indicate me with which API please ?

fdt.*count() or somesuch .
diff mbox

Patch

diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index f3307f4..a6d89a8 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -5,6 +5,7 @@ 
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include "ohci.h"
 
@@ -12,20 +13,98 @@ 
 # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
 #endif
 
+struct ohci_clock {
+	struct clk *clk;
+	struct list_head list;
+};
+
 struct generic_ohci {
 	ohci_t ohci;
+	struct list_head clks;
 };
 
+static int ohci_release_clocks(struct generic_ohci *priv)
+{
+	struct ohci_clock *ohci_clock, *tmp;
+	struct clk *clk;
+	int ret;
+
+	list_for_each_entry_safe(ohci_clock, tmp, &priv->clks, list) {
+		clk = ohci_clock->clk;
+
+		clk_request(clk->dev, clk);
+		if (ret)
+			return ret;
+
+		clk_disable(clk);
+
+		ret = clk_free(clk);
+		if (ret)
+			return ret;
+
+		list_del(&ohci_clock->list);
+	}
+	return 0;
+}
+
 static int ohci_usb_probe(struct udevice *dev)
 {
 	struct ohci_regs *regs = (struct ohci_regs *)dev_get_addr(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int i, ret;
+
+	INIT_LIST_HEAD(&priv->clks);
+
+	for (i = 0; ; i++) {
+		struct ohci_clock *ohci_clock;
+		struct clk *clk;
+
+		clk = devm_kmalloc(dev, sizeof(*clk), GFP_KERNEL);
+		if (!clk) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+
+		ret = clk_get_by_index(dev, i, clk);
+		if (ret < 0)
+			break;
+
+		if (clk_enable(clk)) {
+			error("failed to enable ohci_clock %d\n", i);
+			clk_free(clk);
+			goto clk_err;
+		}
+		clk_free(clk);
+
+		/*
+		 * add enabled clocks into clks list in order to be disabled
+		 * later on ohci_usb_remove() call or in error path if needed
+		 */
+		ohci_clock = devm_kmalloc(dev, sizeof(*ohci_clock), GFP_KERNEL);
+		if (!ohci_clock) {
+			error("Can't allocate resource\n");
+			goto clk_err;
+		}
+		ohci_clock->clk = clk;
+		list_add(&ohci_clock->list, &priv->clks);
+	}
 
 	return ohci_register(dev, regs);
+
+clk_err:
+	return ohci_release_clocks(priv);
 }
 
 static int ohci_usb_remove(struct udevice *dev)
 {
-	return ohci_deregister(dev);
+	struct generic_ohci *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = ohci_deregister(dev);
+	if (ret)
+		return ret;
+
+	return ohci_release_clocks(priv);
 }
 
 static const struct udevice_id ohci_usb_ids[] = {