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.
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
