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

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut