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

[Marek Vasut]

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 at st.com wrote:
>>> From: Patrice Chotard <patrice.chotard at 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 at 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[] = {
>>>
>>


-- 
Best regards,
Marek Vasut