Hi Lothar
On 06/20/2017 01:32 PM, Lothar Waßmann wrote:
> Hi,
>
> On Tue, 20 Jun 2017 11:59:07 +0200 patrice.chot...@st.com wrote:
>> From: Patrice Chotard <patrice.chot...@st.com>
>>
>> Use an array to save enabled clocks reference and deasserted resets
>> in order to respectively disabled and asserted them in case of error
>> during probe() or during driver removal.
>>
>> Signed-off-by: Patrice Chotard <patrice.chot...@st.com>
>> ---
>>
>> v7: _ replace clk_count() and reset_count() by
>> ofnode_count_phandle_with_args()
>>
>> v6: _ none
>>
>> v5: _ none
>>
>> v4: _ update the memory allocation for deasserted resets and enabled
>> clocks reference list. Replace lists by arrays.
>> _ usage of new RESET and CLOCK methods clk_count(), reset_count(),
>> reset_assert_all() and clk_disable_all().
>>
>> v3: _ keep enabled clocks and deasserted resets reference in list in order
>> to
>> disable clock or assert resets in error path or in .remove callback
>> _ use struct generic_ehci * instead of struct udevice * as parameter for
>> ehci_release_resets() and ehci_release_clocks()
>>
>> drivers/usb/host/ehci-generic.c | 125
>> ++++++++++++++++++++++++++++++++--------
>> 1 file changed, 101 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c
>> b/drivers/usb/host/ehci-generic.c
>> index 2116ae1..388b242 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -11,6 +11,8 @@
>> #include <dm.h>
>> #include "ehci.h"
>>
>> +#include <dm/ofnode.h>
>> +
>> /*
>> * Even though here we don't explicitly use "struct ehci_ctrl"
>> * ehci_register() expects it to be the first thing that resides in
>> @@ -18,43 +20,119 @@
>> */
>> struct generic_ehci {
>> struct ehci_ctrl ctrl;
>> + struct clk *clocks;
>> + struct reset_ctl *resets;
>> + int clock_count;
>> + int reset_count;
>> };
>>
>> static int ehci_usb_probe(struct udevice *dev)
>> {
>> + struct generic_ehci *priv = dev_get_priv(dev);
>> struct ehci_hccr *hccr;
>> struct ehci_hcor *hcor;
>> - int i;
>> -
>> - for (i = 0; ; i++) {
>> - struct clk clk;
>> - int ret;
>> -
>> - ret = clk_get_by_index(dev, i, &clk);
>> - if (ret < 0)
>> - break;
>> - if (clk_enable(&clk))
>> - error("failed to enable clock %d\n", i);
>> - clk_free(&clk);
>> - }
>> + int i, err, ret, clock_nb, reset_nb;
>> +
>> + err = 0;
>> + priv->clock_count = 0;
>> + clock_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "clocks",
>> + "#clock-cells");
>> + if (clock_nb > 0) {
>> + priv->clocks = devm_kmalloc(dev, sizeof(struct clk) * clock_nb,
>> + GFP_KERNEL);
>> + if (!priv->clocks) {
>> + error("Can't allocate resource\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (i = 0; i < clock_nb; i++) {
>> + err = clk_get_by_index(dev, i, &priv->clocks[i]);
>> +
>> + if (err < 0)
>> + break;
>>
>> - for (i = 0; ; i++) {
>> - struct reset_ctl reset;
>> - int ret;
>> + priv->clock_count++;
>>
>> - ret = reset_get_by_index(dev, i, &reset);
>> - if (ret < 0)
>> - break;
>> - if (reset_deassert(&reset))
>> - error("failed to deassert reset %d\n", i);
>> - reset_free(&reset);
>> + if (clk_enable(&priv->clocks[i])) {
>> + error("failed to enable clock %d\n", i);
>> + clk_free(&priv->clocks[i]);
>>
> You free the clock here and again with clk_disable_all() in the error
> path. Also you do not have a valid error code in 'err' which is
> returned at the end of the function!
Good catch, to fix it, i will move the priv->clock_count++ after the
clk_enable statement. In this case, in the error path, only clocks which
have been previously correctly enabled will be requested/disabled/freed
Ok i will catch the error code in err and use it as return value.
>
>> + goto clk_err;
>> + }
>> + clk_free(&priv->clocks[i]);
>> + }
>> + } else {
>> + if (clock_nb != -ENOENT) {
>> + error("failed to get clock phandle(%d)\n", clock_nb);
>> + return clock_nb;
>> + }
>> }
>>
>> + priv->reset_count = 0;
>> + reset_nb = ofnode_count_phandle_with_args(dev_ofnode(dev), "resets",
>> + "#reset-cells");
>> + if (reset_nb > 0) {
>> + priv->resets = devm_kmalloc(dev,
>> + sizeof(struct reset_ctl) * reset_nb,
>> + GFP_KERNEL);
>> + if (!priv->resets) {
>> + error("Can't allocate resource\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (i = 0; i < reset_nb; i++) {
>> + err = reset_get_by_index(dev, i, &priv->resets[i]);
>> + if (err < 0)
>> + break;
>> +
>> + priv->reset_count++;
>> +
>> + if (reset_deassert(&priv->resets[i])) {
>> + error("failed to deassert reset %d\n", i);
>> + reset_free(&priv->resets[i]);
>> + goto reset_err;
>> + }
>> + reset_free(&priv->resets[i]);
>> + }
>> + } else {
>> + if (reset_nb != -ENOENT) {
>> + error("failed to get reset phandle(%d)\n", reset_nb);
>> + goto clk_err;
>> + }
>> + }
>> hccr = map_physmem(devfdt_get_addr(dev), 0x100, MAP_NOCACHE);
>> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>>
>> - return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> + err = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
>> + if (!err)
>> + return err;
>> +
>> +reset_err:
>> + ret = reset_assert_all(priv->resets, priv->reset_count);
>> + if (ret)
>> + return ret;
>>
> You should do the clock cleanup even though the reset cleanup failed.
Agree, i will fix it
>
>> +clk_err:
>> + ret = clk_disable_all(priv->clocks, priv->clock_count);
>> + if (ret)
>> + return ret;
>>
> You should return the error code of the operation that failed
> initially (err), not some error code that has occured during cleanup.
Agree again
Thanks
Patrice
>
>> + return err;
>> +}
>> +
>> +static int ehci_usb_remove(struct udevice *dev)
>> +{
>> + struct generic_ehci *priv = dev_get_priv(dev);
>> + int ret;
>> +
>> + ret = ehci_deregister(dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = reset_assert_all(priv->resets, priv->reset_count);
>> + if (ret)
>> + return ret;
>> +
>> + return clk_disable_all(priv->clocks, priv->clock_count);
>> }
>>
>> static const struct udevice_id ehci_usb_ids[] = {
>> @@ -67,7 +145,7 @@ U_BOOT_DRIVER(ehci_generic) = {
>> .id = UCLASS_USB,
>> .of_match = ehci_usb_ids,
>> .probe = ehci_usb_probe,
>> - .remove = ehci_deregister,
>> + .remove = ehci_usb_remove,
>> .ops = &ehci_usb_ops,
>> .priv_auto_alloc_size = sizeof(struct generic_ehci),
>> .flags = DM_FLAG_ALLOC_PRIV_DMA,
>
>
> Lothar Waßmann
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
