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.chot...@st.com wrote:
>>>> From: Patrice Chotard <patrice.chot...@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.chot...@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[] = {
>>>>
>>>
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
