[U-Boot] [PATCH v2 0/5] usb: Extend ehci and ohci generic drivers
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(-)
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
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
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,
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;}
- 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;}@@ -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,
Hi Marek
On 05/12/2017 10:50 PM, Marek Vasut wrote:
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.
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;}
- 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;}@@ -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,
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
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
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);
On 05/12/2017 07:27 PM, 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))
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);
Hi Marek
On 05/12/2017 10:50 PM, Marek Vasut wrote:
On 05/12/2017 07:27 PM, 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))
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);
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
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
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; }
On 05/12/2017 07:27 PM, patrice.chotard@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;
On 12 May 2017 at 14:51, Marek Vasut marex@denx.de wrote:
On 05/12/2017 07:27 PM, patrice.chotard@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
Hi Marek
On 05/12/2017 10:51 PM, Marek Vasut wrote:
On 05/12/2017 07:27 PM, patrice.chotard@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;
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); }
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
participants (4)
-
Marek Vasut -
Patrice CHOTARD -
patrice.chotard＠st.com
-
Simon Glass