Re: [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic

On 05/12/2017 07:27 PM, patrice.chotard@st.com wrote:

From: Patrice Chotard patrice.chotard@st.com

Add error path to disable enabled clocks and to assert deasserted resets Populate the remove callback

Signed-off-by: Patrice Chotard patrice.chotard@st.com

v2: _ split previous path 1, add error path and .remove callback

drivers/usb/host/ehci-generic.c | 81 +++++++++++++++++++++++++++++++++++------ 1 file changed, 70 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index 2190adb..0c29f63 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -11,6 +11,9 @@ #include <dm.h> #include "ehci.h"

+#define EHCI_MAX_CLOCKS 3 +#define EHCI_MAX_RESETS 3

And then someone invents controller with four resets and this whole thing ... breaks.

/*

• Even though here we don't explicitly use "struct ehci_ctrl"
• ehci_register() expects it to be the first thing that resides in

@@ -18,35 +21,74 @@ */ struct generic_ehci { struct ehci_ctrl ctrl;

• struct clk clks[EHCI_MAX_CLOCKS];
• struct reset_ctl resets[EHCI_MAX_RESETS];

};

+static void ehci_assert_resets(struct udevice *dev) {

• struct generic_ehci *priv = dev_get_priv(dev);
• struct reset_ctl reset;
• int i;
• for (i = EHCI_MAX_RESETS; i >= 0; --i) {

Why start from the end ?

• reset = priv->resets[i];
  

• if (reset.dev) {
  

•                    reset_request(&reset);
  

•                    reset_assert(&reset);
  

•                    reset_free(&reset);
  

• }
  

• }

+}

+static void ehci_disable_clocks(struct udevice *dev) {

• struct generic_ehci *priv = dev_get_priv(dev);
• struct clk clk;
• int i;
• for (i = EHCI_MAX_CLOCKS; i >= 0; --i) {

• clk = priv->clks[i];
  

• if (clk.dev) {
  

• 	clk_request(clk.dev, &clk);
  

• 	clk_disable(&clk);
  

• 	clk_free(&clk);
  

• }
  

• }

+}

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;

• int i, ret;

• for (i = 0; ; i++) {

• struct clk clk;
  

• int ret;
  

• for (i = 0; i < EHCI_MAX_CLOCKS; i++) {

• struct clk clk = priv->clks[i];
  
  

  ret = clk_get_by_index(dev, i, &clk); if (ret < 0) break;

• if (clk_enable(&clk))
  

• if (clk_enable(&clk)) {
  printf("failed to enable clock %d\n", i);
  

• 	clk_free(&clk);
  

• 	goto clk_err;
  

• }
  

  clk_free(&clk); }

• for (i = 0; ; i++) {

• struct reset_ctl reset;
  

• int ret;
  

• for (i = 0; i < EHCI_MAX_RESETS ; i++) {

• struct reset_ctl reset = priv->resets[i];
  
  

  ret = reset_get_by_index(dev, i, &reset); if (ret < 0) break;

• if (reset_deassert(&reset))
  

• if (reset_deassert(&reset)) {
  printf("failed to deassert reset %d\n", i);
  

• 	reset_free(&reset);
  

• 	goto reset_err;
  

• }
  

  reset_free(&reset); }

@@ -54,7 +96,24 @@ static int ehci_usb_probe(struct udevice *dev) 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);

• ret = ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
• if (!ret)

• return ret;
  

+reset_err:

• ehci_assert_resets(dev);

+clk_err:

• ehci_disable_clocks(dev);
• return ret;

+}

+static int ehci_usb_remove(struct udevice *dev) {

• ehci_assert_resets(dev);
• ehci_disable_clocks(dev);
• return ehci_deregister(dev);

}

static const struct udevice_id ehci_usb_ids[] = { @@ -67,7 +126,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,