* [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers
@ 2017-05-12 17:27 patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 1/5] reset: add reset_request() patrice.chotard at st.com
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
From: Patrice Chotard <patrice.chotard@st.com>
v2: _ add needed reset_request() in RESET framework
_ add error path in ehci/ohci-generic to disable clocks and to assert
resets
_ add .remove callback with clocks, resets and phy release
_ split the replacement of printf() by error() in an independant patch
Patrice Chotard (5):
reset: add reset_request()
usb: host: add error path and remove callback in ehci-generic
usb: host: extend generic EHCI driver with PHY
usb: host: replace printf() by error() in ehci-generic
usb: host: extend generic OHCI with CLOCK, RESET and PHY
drivers/reset/reset-uclass.c | 9 ++++
drivers/usb/host/ehci-generic.c | 96 +++++++++++++++++++++++++++++++++------
drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
include/reset.h | 9 ++++
4 files changed, 197 insertions(+), 16 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 1/5] reset: add reset_request()
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
@ 2017-05-12 17:27 ` patrice.chotard at st.com
2017-05-15 3:03 ` Simon Glass
2017-05-12 17:27 ` [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic patrice.chotard at st.com
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
From: Patrice Chotard <patrice.chotard@st.com>
This is needed in error path to assert previously deasserted
reset by using a saved reset_ctl reference.
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
drivers/reset/reset-uclass.c | 9 +++++++++
include/reset.h | 9 +++++++++
2 files changed, 18 insertions(+)
diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index e92b24f..916f210 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -98,6 +98,15 @@ int reset_get_by_name(struct udevice *dev, const char *name,
return reset_get_by_index(dev, index, reset_ctl);
}
+int reset_request(struct reset_ctl *reset_ctl)
+{
+ struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+
+ debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+
+ return ops->request(reset_ctl);
+}
+
int reset_free(struct reset_ctl *reset_ctl)
{
struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
diff --git a/include/reset.h b/include/reset.h
index f45fcf8..4f2e35f 100644
--- a/include/reset.h
+++ b/include/reset.h
@@ -100,6 +100,15 @@ int reset_get_by_name(struct udevice *dev, const char *name,
struct reset_ctl *reset_ctl);
/**
+ * reset_request - Request a reset signal.
+ *
+ * @reset_ctl: A reset control struct.
+ *
+ * @return 0 if OK, or a negative error code.
+ */
+int reset_request(struct reset_ctl *reset_ctl);
+
+/**
* reset_free - Free a previously requested reset signal.
*
* @reset_ctl: A reset control struct that was previously successfully
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 1/5] reset: add reset_request() patrice.chotard at st.com
@ 2017-05-12 17:27 ` patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
2017-05-15 3:03 ` Simon Glass
2017-05-12 17:27 ` [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY patrice.chotard at st.com
` (2 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
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
+
/*
* 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) {
+ 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,
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 1/5] reset: add reset_request() patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic patrice.chotard at st.com
@ 2017-05-12 17:27 ` patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
2017-05-15 3:03 ` Simon Glass
2017-05-12 17:27 ` [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY patrice.chotard at st.com
4 siblings, 2 replies; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
From: Patrice Chotard <patrice.chotard@st.com>
Add support of generic PHY framework
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
v2: _ split previous path 1, add generic PHY framework
drivers/usb/host/ehci-generic.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 0c29f63..0000808 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -6,6 +6,8 @@
#include <common.h>
#include <clk.h>
+#include <fdtdec.h>
+#include <generic-phy.h>
#include <reset.h>
#include <asm/io.h>
#include <dm.h>
@@ -23,6 +25,7 @@ struct generic_ehci {
struct ehci_ctrl ctrl;
struct clk clks[EHCI_MAX_CLOCKS];
struct reset_ctl resets[EHCI_MAX_RESETS];
+ struct phy phy;
};
static void ehci_assert_resets(struct udevice *dev) {
@@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
reset_free(&reset);
}
+ if (!generic_phy_get_by_index(dev, 0, &priv->phy))
+ if (generic_phy_init(&priv->phy))
+ error("failed to init usb phy %d\n", i);
+
hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
hcor = (struct ehci_hcor *)((uintptr_t)hccr +
HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
@@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
if (!ret)
return ret;
+ generic_phy_exit(&priv->phy);
+
reset_err:
ehci_assert_resets(dev);
clk_err:
@@ -109,7 +118,9 @@ clk_err:
}
static int ehci_usb_remove(struct udevice *dev) {
+ struct generic_ehci *priv = dev_get_priv(dev);
+ generic_phy_exit(&priv->phy);
ehci_assert_resets(dev);
ehci_disable_clocks(dev);
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
` (2 preceding siblings ...)
2017-05-12 17:27 ` [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY patrice.chotard at st.com
@ 2017-05-12 17:27 ` patrice.chotard at st.com
2017-05-12 20:51 ` Marek Vasut
2017-05-12 17:27 ` [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY patrice.chotard at st.com
4 siblings, 1 reply; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
From: Patrice Chotard <patrice.chotard@st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
v2: _ create this independant path for printf() replacement
drivers/usb/host/ehci-generic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index 0000808..39b56de 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
if (ret < 0)
break;
if (clk_enable(&clk)) {
- printf("failed to enable clock %d\n", i);
+ error("failed to enable clock %d\n", i);
clk_free(&clk);
goto clk_err;
}
@@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
if (ret < 0)
break;
if (reset_deassert(&reset)) {
- printf("failed to deassert reset %d\n", i);
+ error("failed to deassert reset %d\n", i);
reset_free(&reset);
goto reset_err;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
` (3 preceding siblings ...)
2017-05-12 17:27 ` [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic patrice.chotard at st.com
@ 2017-05-12 17:27 ` patrice.chotard at st.com
2017-05-15 3:27 ` Simon Glass
4 siblings, 1 reply; 19+ messages in thread
From: patrice.chotard at st.com @ 2017-05-12 17:27 UTC (permalink / raw)
To: u-boot
From: Patrice Chotard <patrice.chotard@st.com>
Add CLOCK, RESET and generic PHY frameworks support
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
v2: _ add error path management
_ add .remove callback
drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 96 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
index f3307f4..f86e223 100644
--- a/drivers/usb/host/ohci-generic.c
+++ b/drivers/usb/host/ohci-generic.c
@@ -5,26 +5,119 @@
*/
#include <common.h>
+#include <clk.h>
#include <dm.h>
+#include <fdtdec.h>
+#include <generic-phy.h>
+#include <reset.h>
+
#include "ohci.h"
#if !defined(CONFIG_USB_OHCI_NEW)
# error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
#endif
+#define OHCI_MAX_CLOCKS 3
+#define OHCI_MAX_RESETS 3
+
struct generic_ohci {
ohci_t ohci;
+ struct clk clks[OHCI_MAX_CLOCKS];
+ struct reset_ctl resets[OHCI_MAX_RESETS];
+ struct phy phy;
};
+static void ohci_assert_resets(struct udevice *dev) {
+ struct generic_ohci *priv = dev_get_priv(dev);
+ struct reset_ctl reset;
+ int i;
+
+ for (i = OHCI_MAX_RESETS; i >= 0; --i) {
+ reset = priv->resets[i];
+
+ if (reset.dev) {
+ reset_request(&reset);
+ reset_assert(&reset);
+ reset_free(&reset);
+ }
+ }
+}
+
+static void ohci_disable_clocks(struct udevice *dev) {
+ struct generic_ohci *priv = dev_get_priv(dev);
+ struct clk clk;
+ int i;
+
+ for (i = OHCI_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 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;
+
+ for (i = 0; i < OHCI_MAX_CLOCKS; i++) {
+ struct clk clk = priv->clks[i];
- return ohci_register(dev, regs);
+ 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);
+ goto clk_err;
+ }
+ clk_free(&clk);
+ }
+
+ for (i = 0; i < OHCI_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)) {
+ error("failed to deassert reset %d\n", i);
+ reset_free(&reset);
+ goto reset_err;
+ }
+ reset_free(&reset);
+ }
+
+ if (!generic_phy_get_by_index(dev, 0, &priv->phy))
+ if (generic_phy_init(&priv->phy))
+ error("failed to init usb phy %d\n", i);
+
+ ret = ohci_register(dev, regs);
+ if (!ret)
+ return ret;
+
+ generic_phy_exit(&priv->phy);
+
+reset_err:
+ ohci_assert_resets(dev);
+clk_err:
+ ohci_disable_clocks(dev);
+
+ return ret;
}
-static int ohci_usb_remove(struct udevice *dev)
-{
+static int ohci_usb_remove(struct udevice *dev) {
+ struct generic_ohci *priv = dev_get_priv(dev);
+
+ generic_phy_exit(&priv->phy);
+ ohci_assert_resets(dev);
+ ohci_disable_clocks(dev);
+
return ohci_deregister(dev);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic
2017-05-12 17:27 ` [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic patrice.chotard at st.com
@ 2017-05-12 20:50 ` Marek Vasut
2017-05-15 7:56 ` Patrice CHOTARD
2017-05-15 3:03 ` Simon Glass
1 sibling, 1 reply; 19+ messages in thread
From: Marek Vasut @ 2017-05-12 20:50 UTC (permalink / raw)
To: u-boot
On 05/12/2017 07:27 PM, patrice.chotard at 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,
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY
2017-05-12 17:27 ` [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY patrice.chotard at st.com
@ 2017-05-12 20:50 ` Marek Vasut
2017-05-15 7:59 ` Patrice CHOTARD
2017-05-15 3:03 ` Simon Glass
1 sibling, 1 reply; 19+ messages in thread
From: Marek Vasut @ 2017-05-12 20:50 UTC (permalink / raw)
To: u-boot
On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add support of generic PHY framework
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2: _ split previous path 1, add generic PHY framework
>
> drivers/usb/host/ehci-generic.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0c29f63..0000808 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -6,6 +6,8 @@
>
> #include <common.h>
> #include <clk.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
> #include <reset.h>
> #include <asm/io.h>
> #include <dm.h>
> @@ -23,6 +25,7 @@ struct generic_ehci {
> struct ehci_ctrl ctrl;
> struct clk clks[EHCI_MAX_CLOCKS];
> struct reset_ctl resets[EHCI_MAX_RESETS];
> + struct phy phy;
> };
>
> static void ehci_assert_resets(struct udevice *dev) {
> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
> reset_free(&reset);
> }
>
> + if (!generic_phy_get_by_index(dev, 0, &priv->phy))
> + if (generic_phy_init(&priv->phy))
> + error("failed to init usb phy %d\n", i);
> +
> hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
> if (!ret)
> return ret;
>
> + generic_phy_exit(&priv->phy);
So you probe the EHCI controller driver and then you disable it's PHY ?
That's a bit odd, isn't it ?
> reset_err:
> ehci_assert_resets(dev);
> clk_err:
> @@ -109,7 +118,9 @@ clk_err:
> }
>
> static int ehci_usb_remove(struct udevice *dev) {
> + struct generic_ehci *priv = dev_get_priv(dev);
>
> + generic_phy_exit(&priv->phy);
> ehci_assert_resets(dev);
> ehci_disable_clocks(dev);
>
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic
2017-05-12 17:27 ` [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic patrice.chotard at st.com
@ 2017-05-12 20:51 ` Marek Vasut
2017-05-15 3:27 ` Simon Glass
2017-05-15 8:00 ` Patrice CHOTARD
0 siblings, 2 replies; 19+ messages in thread
From: Marek Vasut @ 2017-05-12 20:51 UTC (permalink / raw)
To: u-boot
On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
Commit message does not explain WHY this change is needed. In fact ...
commit message is missing altogether ...
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2: _ create this independant path for printf() replacement
>
> drivers/usb/host/ehci-generic.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0000808..39b56de 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
> if (ret < 0)
> break;
> if (clk_enable(&clk)) {
> - printf("failed to enable clock %d\n", i);
> + error("failed to enable clock %d\n", i);
> clk_free(&clk);
> goto clk_err;
> }
> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
> if (ret < 0)
> break;
> if (reset_deassert(&reset)) {
> - printf("failed to deassert reset %d\n", i);
> + error("failed to deassert reset %d\n", i);
> reset_free(&reset);
> goto reset_err;
> }
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 1/5] reset: add reset_request()
2017-05-12 17:27 ` [U-Boot] [PATCH v2 1/5] reset: add reset_request() patrice.chotard at st.com
@ 2017-05-15 3:03 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2017-05-15 3:03 UTC (permalink / raw)
To: u-boot
On 12 May 2017 at 11:27, <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> This is needed in error path to assert previously deasserted
> reset by using a saved reset_ctl reference.
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> drivers/reset/reset-uclass.c | 9 +++++++++
> include/reset.h | 9 +++++++++
> 2 files changed, 18 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic
2017-05-12 17:27 ` [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
@ 2017-05-15 3:03 ` Simon Glass
2017-05-15 8:02 ` Patrice CHOTARD
1 sibling, 1 reply; 19+ messages in thread
From: Simon Glass @ 2017-05-15 3:03 UTC (permalink / raw)
To: u-boot
Hi Patrice,
On 12 May 2017 at 11:27, <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(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
Please see below.
>
> 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
> +
> /*
> * 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);
You could pass priv instead of dev to avoid this line.
> + struct reset_ctl reset;
> + int i;
> +
> + for (i = EHCI_MAX_RESETS; i >= 0; --i) {
> + 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) {
Do you think it is worth adding a function in the uclass to do this to
a list of clks?
> + 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,
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY
2017-05-12 17:27 ` [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
@ 2017-05-15 3:03 ` Simon Glass
2017-05-15 8:03 ` Patrice CHOTARD
1 sibling, 1 reply; 19+ messages in thread
From: Simon Glass @ 2017-05-15 3:03 UTC (permalink / raw)
To: u-boot
Hi Patrice,
On 12 May 2017 at 11:27, <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add support of generic PHY framework
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2: _ split previous path 1, add generic PHY framework
>
> drivers/usb/host/ehci-generic.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> index 0c29f63..0000808 100644
> --- a/drivers/usb/host/ehci-generic.c
> +++ b/drivers/usb/host/ehci-generic.c
> @@ -6,6 +6,8 @@
>
> #include <common.h>
> #include <clk.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
> #include <reset.h>
> #include <asm/io.h>
> #include <dm.h>
> @@ -23,6 +25,7 @@ struct generic_ehci {
> struct ehci_ctrl ctrl;
> struct clk clks[EHCI_MAX_CLOCKS];
> struct reset_ctl resets[EHCI_MAX_RESETS];
> + struct phy phy;
> };
>
> static void ehci_assert_resets(struct udevice *dev) {
> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
> reset_free(&reset);
> }
>
> + if (!generic_phy_get_by_index(dev, 0, &priv->phy))
Can you check for the error you expect here when it is not present? I
think it might be -ENOENT? That way you are not ignoring a real error.
> + if (generic_phy_init(&priv->phy))
> + error("failed to init usb phy %d\n", i);
Shouldn't you return the error here? I don't think USB will work
without the PHY.
> +
> hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
> if (!ret)
> return ret;
>
> + generic_phy_exit(&priv->phy);
> +
> reset_err:
> ehci_assert_resets(dev);
> clk_err:
> @@ -109,7 +118,9 @@ clk_err:
> }
>
> static int ehci_usb_remove(struct udevice *dev) {
> + struct generic_ehci *priv = dev_get_priv(dev);
>
> + generic_phy_exit(&priv->phy);
> ehci_assert_resets(dev);
> ehci_disable_clocks(dev);
>
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic
2017-05-12 20:51 ` Marek Vasut
@ 2017-05-15 3:27 ` Simon Glass
2017-05-15 8:00 ` Patrice CHOTARD
1 sibling, 0 replies; 19+ messages in thread
From: Simon Glass @ 2017-05-15 3:27 UTC (permalink / raw)
To: u-boot
On 12 May 2017 at 14:51, Marek Vasut <marex@denx.de> wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>
> Commit message does not explain WHY this change is needed. In fact ...
> commit message is missing altogether ...
Yes it really helps to have a commit message!
>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2: _ create this independant path for printf() replacement
Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> drivers/usb/host/ehci-generic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0000808..39b56de 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (ret < 0)
>> break;
>> if (clk_enable(&clk)) {
>> - printf("failed to enable clock %d\n", i);
>> + error("failed to enable clock %d\n", i);
>> clk_free(&clk);
>> goto clk_err;
>> }
>> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (ret < 0)
>> break;
>> if (reset_deassert(&reset)) {
>> - printf("failed to deassert reset %d\n", i);
>> + error("failed to deassert reset %d\n", i);
>> reset_free(&reset);
>> goto reset_err;
>> }
>>
>
>
> --
> Best regards,
> Marek Vasut
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY
2017-05-12 17:27 ` [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY patrice.chotard at st.com
@ 2017-05-15 3:27 ` Simon Glass
0 siblings, 0 replies; 19+ messages in thread
From: Simon Glass @ 2017-05-15 3:27 UTC (permalink / raw)
To: u-boot
On 12 May 2017 at 11:27, <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Add CLOCK, RESET and generic PHY frameworks support
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
>
> v2: _ add error path management
> _ add .remove callback
>
> drivers/usb/host/ohci-generic.c | 99 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index f3307f4..f86e223 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -5,26 +5,119 @@
> */
>
> #include <common.h>
> +#include <clk.h>
> #include <dm.h>
> +#include <fdtdec.h>
> +#include <generic-phy.h>
> +#include <reset.h>
> +
> #include "ohci.h"
>
> #if !defined(CONFIG_USB_OHCI_NEW)
> # error "Generic OHCI driver requires CONFIG_USB_OHCI_NEW"
> #endif
>
> +#define OHCI_MAX_CLOCKS 3
> +#define OHCI_MAX_RESETS 3
> +
> struct generic_ohci {
> ohci_t ohci;
> + struct clk clks[OHCI_MAX_CLOCKS];
> + struct reset_ctl resets[OHCI_MAX_RESETS];
> + struct phy phy;
> };
>
> +static void ohci_assert_resets(struct udevice *dev) {
> + struct generic_ohci *priv = dev_get_priv(dev);
> + struct reset_ctl reset;
> + int i;
> +
> + for (i = OHCI_MAX_RESETS; i >= 0; --i) {
> + reset = priv->resets[i];
> +
> + if (reset.dev) {
> + reset_request(&reset);
> + reset_assert(&reset);
> + reset_free(&reset);
Don't you need error checking here?
> + }
> + }
> +}
> +
> +static void ohci_disable_clocks(struct udevice *dev) {
> + struct generic_ohci *priv = dev_get_priv(dev);
> + struct clk clk;
> + int i;
> +
> + for (i = OHCI_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 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;
> +
> + for (i = 0; i < OHCI_MAX_CLOCKS; i++) {
> + struct clk clk = priv->clks[i];
>
> - return ohci_register(dev, regs);
> + 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);
> + goto clk_err;
> + }
> + clk_free(&clk);
> + }
> +
> + for (i = 0; i < OHCI_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)) {
> + error("failed to deassert reset %d\n", i);
> + reset_free(&reset);
> + goto reset_err;
> + }
> + reset_free(&reset);
> + }
> +
> + if (!generic_phy_get_by_index(dev, 0, &priv->phy))
> + if (generic_phy_init(&priv->phy))
> + error("failed to init usb phy %d\n", i);
> +
> + ret = ohci_register(dev, regs);
> + if (!ret)
> + return ret;
> +
> + generic_phy_exit(&priv->phy);
> +
> +reset_err:
> + ohci_assert_resets(dev);
> +clk_err:
> + ohci_disable_clocks(dev);
> +
> + return ret;
> }
>
> -static int ohci_usb_remove(struct udevice *dev)
> -{
> +static int ohci_usb_remove(struct udevice *dev) {
> + struct generic_ohci *priv = dev_get_priv(dev);
> +
> + generic_phy_exit(&priv->phy);
> + ohci_assert_resets(dev);
> + ohci_disable_clocks(dev);
> +
> return ohci_deregister(dev);
> }
>
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic
2017-05-12 20:50 ` Marek Vasut
@ 2017-05-15 7:56 ` Patrice CHOTARD
0 siblings, 0 replies; 19+ messages in thread
From: Patrice CHOTARD @ 2017-05-15 7:56 UTC (permalink / raw)
To: u-boot
Hi Marek
On 05/12/2017 10:50 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at 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.
OK i will use a more generic solution based on list
>
>> /*
>> * 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 ?
Simply to assert resets in reverse order they have been deasserted.
Is it a problematic ?
Thanks
Patrice
>
>> + 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,
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY
2017-05-12 20:50 ` Marek Vasut
@ 2017-05-15 7:59 ` Patrice CHOTARD
0 siblings, 0 replies; 19+ messages in thread
From: Patrice CHOTARD @ 2017-05-15 7:59 UTC (permalink / raw)
To: u-boot
Hi Marek
On 05/12/2017 10:50 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add support of generic PHY framework
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2: _ split previous path 1, add generic PHY framework
>>
>> drivers/usb/host/ehci-generic.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0c29f63..0000808 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -6,6 +6,8 @@
>>
>> #include <common.h>
>> #include <clk.h>
>> +#include <fdtdec.h>
>> +#include <generic-phy.h>
>> #include <reset.h>
>> #include <asm/io.h>
>> #include <dm.h>
>> @@ -23,6 +25,7 @@ struct generic_ehci {
>> struct ehci_ctrl ctrl;
>> struct clk clks[EHCI_MAX_CLOCKS];
>> struct reset_ctl resets[EHCI_MAX_RESETS];
>> + struct phy phy;
>> };
>>
>> static void ehci_assert_resets(struct udevice *dev) {
>> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>> reset_free(&reset);
>> }
>>
>> + if (!generic_phy_get_by_index(dev, 0, &priv->phy))
>> + if (generic_phy_init(&priv->phy))
>> + error("failed to init usb phy %d\n", i);
>> +
>> hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (!ret)
>> return ret;
>>
>> + generic_phy_exit(&priv->phy);
>
> So you probe the EHCI controller driver and then you disable it's PHY ?
> That's a bit odd, isn't it ?
The PHY is disabled in case of ehci_register() didn't succeed.
Patrice
>
>> reset_err:
>> ehci_assert_resets(dev);
>> clk_err:
>> @@ -109,7 +118,9 @@ clk_err:
>> }
>>
>> static int ehci_usb_remove(struct udevice *dev) {
>> + struct generic_ehci *priv = dev_get_priv(dev);
>>
>> + generic_phy_exit(&priv->phy);
>> ehci_assert_resets(dev);
>> ehci_disable_clocks(dev);
>>
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic
2017-05-12 20:51 ` Marek Vasut
2017-05-15 3:27 ` Simon Glass
@ 2017-05-15 8:00 ` Patrice CHOTARD
1 sibling, 0 replies; 19+ messages in thread
From: Patrice CHOTARD @ 2017-05-15 8:00 UTC (permalink / raw)
To: u-boot
Hi Marek
On 05/12/2017 10:51 PM, Marek Vasut wrote:
> On 05/12/2017 07:27 PM, patrice.chotard at st.com wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>
> Commit message does not explain WHY this change is needed. In fact ...
> commit message is missing altogether ...
Yes, my bad i forget it, i will add it in the v3
Thanks
Patrice
>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2: _ create this independant path for printf() replacement
>>
>> drivers/usb/host/ehci-generic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0000808..39b56de 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -74,7 +74,7 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (ret < 0)
>> break;
>> if (clk_enable(&clk)) {
>> - printf("failed to enable clock %d\n", i);
>> + error("failed to enable clock %d\n", i);
>> clk_free(&clk);
>> goto clk_err;
>> }
>> @@ -88,7 +88,7 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (ret < 0)
>> break;
>> if (reset_deassert(&reset)) {
>> - printf("failed to deassert reset %d\n", i);
>> + error("failed to deassert reset %d\n", i);
>> reset_free(&reset);
>> goto reset_err;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic
2017-05-15 3:03 ` Simon Glass
@ 2017-05-15 8:02 ` Patrice CHOTARD
0 siblings, 0 replies; 19+ messages in thread
From: Patrice CHOTARD @ 2017-05-15 8:02 UTC (permalink / raw)
To: u-boot
Hi Simon
On 05/15/2017 05:03 AM, Simon Glass wrote:
> Hi Patrice,
>
> On 12 May 2017 at 11:27, <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(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please see below.
>
>>
>> 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
>> +
>> /*
>> * 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);
>
> You could pass priv instead of dev to avoid this line.
Agree, will fix it
>
>> + struct reset_ctl reset;
>> + int i;
>> +
>> + for (i = EHCI_MAX_RESETS; i >= 0; --i) {
>> + 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) {
>
> Do you think it is worth adding a function in the uclass to do this to
> a list of clks?
I will have a look at this
Patrice
>
>> + 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,
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY
2017-05-15 3:03 ` Simon Glass
@ 2017-05-15 8:03 ` Patrice CHOTARD
0 siblings, 0 replies; 19+ messages in thread
From: Patrice CHOTARD @ 2017-05-15 8:03 UTC (permalink / raw)
To: u-boot
Hi Simon
On 05/15/2017 05:03 AM, Simon Glass wrote:
> Hi Patrice,
>
> On 12 May 2017 at 11:27, <patrice.chotard@st.com> wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Add support of generic PHY framework
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>>
>> v2: _ split previous path 1, add generic PHY framework
>>
>> drivers/usb/host/ehci-generic.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
>> index 0c29f63..0000808 100644
>> --- a/drivers/usb/host/ehci-generic.c
>> +++ b/drivers/usb/host/ehci-generic.c
>> @@ -6,6 +6,8 @@
>>
>> #include <common.h>
>> #include <clk.h>
>> +#include <fdtdec.h>
>> +#include <generic-phy.h>
>> #include <reset.h>
>> #include <asm/io.h>
>> #include <dm.h>
>> @@ -23,6 +25,7 @@ struct generic_ehci {
>> struct ehci_ctrl ctrl;
>> struct clk clks[EHCI_MAX_CLOCKS];
>> struct reset_ctl resets[EHCI_MAX_RESETS];
>> + struct phy phy;
>> };
>>
>> static void ehci_assert_resets(struct udevice *dev) {
>> @@ -92,6 +95,10 @@ static int ehci_usb_probe(struct udevice *dev)
>> reset_free(&reset);
>> }
>>
>> + if (!generic_phy_get_by_index(dev, 0, &priv->phy))
>
> Can you check for the error you expect here when it is not present? I
> think it might be -ENOENT? That way you are not ignoring a real error.
ok
>
>> + if (generic_phy_init(&priv->phy))
>> + error("failed to init usb phy %d\n", i);
>
> Shouldn't you return the error here? I don't think USB will work
> without the PHY.
Agree, i forgot it
Thanks
Patrice
>
>> +
>> hccr = map_physmem(dev_get_addr(dev), 0x100, MAP_NOCACHE);
>> hcor = (struct ehci_hcor *)((uintptr_t)hccr +
>> HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
>> @@ -100,6 +107,8 @@ static int ehci_usb_probe(struct udevice *dev)
>> if (!ret)
>> return ret;
>>
>> + generic_phy_exit(&priv->phy);
>> +
>> reset_err:
>> ehci_assert_resets(dev);
>> clk_err:
>> @@ -109,7 +118,9 @@ clk_err:
>> }
>>
>> static int ehci_usb_remove(struct udevice *dev) {
>> + struct generic_ehci *priv = dev_get_priv(dev);
>>
>> + generic_phy_exit(&priv->phy);
>> ehci_assert_resets(dev);
>> ehci_disable_clocks(dev);
>>
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-05-15 8:03 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-12 17:27 [U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers patrice.chotard at st.com
2017-05-12 17:27 ` [U-Boot] [PATCH v2 1/5] reset: add reset_request() patrice.chotard at st.com
2017-05-15 3:03 ` Simon Glass
2017-05-12 17:27 ` [U-Boot] [PATCH v2 2/5] usb: host: add error path and remove callback in ehci-generic patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
2017-05-15 7:56 ` Patrice CHOTARD
2017-05-15 3:03 ` Simon Glass
2017-05-15 8:02 ` Patrice CHOTARD
2017-05-12 17:27 ` [U-Boot] [PATCH v2 3/5] usb: host: extend generic EHCI driver with PHY patrice.chotard at st.com
2017-05-12 20:50 ` Marek Vasut
2017-05-15 7:59 ` Patrice CHOTARD
2017-05-15 3:03 ` Simon Glass
2017-05-15 8:03 ` Patrice CHOTARD
2017-05-12 17:27 ` [U-Boot] [PATCH v2 4/5] usb: host: replace printf() by error() in ehci-generic patrice.chotard at st.com
2017-05-12 20:51 ` Marek Vasut
2017-05-15 3:27 ` Simon Glass
2017-05-15 8:00 ` Patrice CHOTARD
2017-05-12 17:27 ` [U-Boot] [PATCH v2 5/5] usb: host: extend generic OHCI with CLOCK, RESET and PHY patrice.chotard at st.com
2017-05-15 3:27 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.