[U-Boot] [PATCH v2 0/2] usb: ehci: exynos: Enable non-dt path
Based on 'master' branch of u-boot-samsung.
Changes from v1: - Added patch to fix problem of multiple FDT decode. - Addressing warnings when compiling non-dt way.
Vivek Gautam (2): usb: ehci: exynos: Fix multiple FDT decode usb: ehci: exynos: Enable non-dt path
drivers/usb/host/ehci-exynos.c | 47 +++++++++++++++------------------------ 1 files changed, 18 insertions(+), 29 deletions(-)
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com ---
This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch.
drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb; - unsigned int *hcd; + unsigned int hcd; };
+static struct exynos_ehci exynos; + static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */ - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); - if (exynos->hcd == NULL) { + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); + if (exynos->hcd < 0) { debug("Can't get the EHCI register address\n"); return -ENXIO; } @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) { - struct exynos_ehci *exynos = NULL; - - exynos = (struct exynos_ehci *) - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); - if (!exynos) { - debug("failed to allocate exynos ehci context\n"); - return -ENOMEM; - } + struct exynos_ehci *ctx = &exynos;
- exynos_usb_parse_dt(gd->fdt_blob, exynos); + exynos_usb_parse_dt(gd->fdt_blob, ctx);
- setup_usb_phy(exynos->usb); + setup_usb_phy(ctx->usb);
- *hccr = (struct ehci_hccr *)(exynos->hcd); + *hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
@@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
- kfree(exynos); - return 0; }
@@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) { - struct exynos_ehci *exynos = NULL; - - exynos = (struct exynos_ehci *) - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); - if (!exynos) { - debug("failed to allocate exynos ehci context\n"); - return -ENOMEM; - } - - exynos_usb_parse_dt(gd->fdt_blob, exynos); - - reset_usb_phy(exynos->usb); + struct exynos_ehci *ctx = &exynos;
- kfree(exynos); + reset_usb_phy(ctx->usb);
return 0; }
Hi,
On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam gautam.vivek@samsung.com wrote:
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com
Nice patch.
This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch.
drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb;
unsigned int *hcd;
unsigned int hcd;
I think this should be a pointer. Perhaps a (struct ehci_hccr *?
};
+static struct exynos_ehci exynos;
static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */
exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg");if (exynos->hcd == NULL) {
exynos->hcd = fdtdec_get_addr(blob, node, "reg");if (exynos->hcd < 0) {
You need to check for FDT_ADDR_NONE here.
debug("Can't get the EHCI register address\n"); return -ENXIO; }@@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}
struct exynos_ehci *ctx = &exynos;
exynos_usb_parse_dt(gd->fdt_blob, exynos);
exynos_usb_parse_dt(gd->fdt_blob, ctx);
setup_usb_phy(exynos->usb);
setup_usb_phy(ctx->usb);
*hccr = (struct ehci_hccr *)(exynos->hcd);
*hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));@@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
kfree(exynos);return 0;}
@@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}exynos_usb_parse_dt(gd->fdt_blob, exynos);reset_usb_phy(exynos->usb);
struct exynos_ehci *ctx = &exynos;
kfree(exynos);
reset_usb_phy(ctx->usb); return 0;}
1.7.6.5
Regards, Simon
Hi,
On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam gautam.vivek@samsung.com wrote:
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com
Nice patch.
Really sorry for the delay in responding.
This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch.
drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb;
unsigned int *hcd;
unsigned int hcd;I think this should be a pointer. Perhaps a (struct ehci_hccr *?
Isn't it better to maintain it as unsigned int ? Since later when fdtdec_get_addr() is used, which returns u32 or u64, we can easily check against FDT_ADDR_T_NONE. ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed.
};
+static struct exynos_ehci exynos;
static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */
exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg");if (exynos->hcd == NULL) {
exynos->hcd = fdtdec_get_addr(blob, node, "reg");if (exynos->hcd < 0) {You need to check for FDT_ADDR_NONE here.
Sure.
debug("Can't get the EHCI register address\n"); return -ENXIO; }@@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}
struct exynos_ehci *ctx = &exynos;
exynos_usb_parse_dt(gd->fdt_blob, exynos);
exynos_usb_parse_dt(gd->fdt_blob, ctx);
setup_usb_phy(exynos->usb);
setup_usb_phy(ctx->usb);
*hccr = (struct ehci_hccr *)(exynos->hcd);
*hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));@@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
kfree(exynos);return 0;}
@@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}exynos_usb_parse_dt(gd->fdt_blob, exynos);reset_usb_phy(exynos->usb);
struct exynos_ehci *ctx = &exynos;
kfree(exynos);
reset_usb_phy(ctx->usb); return 0;}
1.7.6.5
On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam gautamvivek1987@gmail.com wrote:
Hi,
On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam gautam.vivek@samsung.com wrote:
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com
Nice patch.
Really sorry for the delay in responding.
No problem, we all have other things to do :-)
This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch.
drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb;
unsigned int *hcd;
unsigned int hcd;I think this should be a pointer. Perhaps a (struct ehci_hccr *?
Isn't it better to maintain it as unsigned int ? Since later when fdtdec_get_addr() is used, which returns u32 or u64, we can easily check against FDT_ADDR_T_NONE. ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed.
How about having a local variable in your decode routine like:
fdt_addr_t addr;
addr = fdtdec_get_addr()... if (addr == FDT_ADDR_NONE) { debug(...) return -1; } exynos->hcd = (struct ... *)addr;
I think this is better than holding a value that is the wrong type, and casting it throughout your driver. Best to get the casting/checking done at init time,
};
+static struct exynos_ehci exynos;
static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */
exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg");if (exynos->hcd == NULL) {
exynos->hcd = fdtdec_get_addr(blob, node, "reg");if (exynos->hcd < 0) {You need to check for FDT_ADDR_NONE here.
Sure.
debug("Can't get the EHCI register address\n"); return -ENXIO; }@@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}
struct exynos_ehci *ctx = &exynos;
exynos_usb_parse_dt(gd->fdt_blob, exynos);
exynos_usb_parse_dt(gd->fdt_blob, ctx);
setup_usb_phy(exynos->usb);
setup_usb_phy(ctx->usb);
*hccr = (struct ehci_hccr *)(exynos->hcd);
*hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));@@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
kfree(exynos);return 0;}
@@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}exynos_usb_parse_dt(gd->fdt_blob, exynos);reset_usb_phy(exynos->usb);
struct exynos_ehci *ctx = &exynos;
kfree(exynos);
reset_usb_phy(ctx->usb); return 0;}
1.7.6.5
-- Thanks & Regards Vivek
Regards, Simon
Hi,
On Wed, Mar 6, 2013 at 7:27 AM, Simon Glass sjg@chromium.org wrote:
On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam gautamvivek1987@gmail.com wrote:
Hi,
On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam gautam.vivek@samsung.com wrote:
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com
Nice patch.
Really sorry for the delay in responding.
No problem, we all have other things to do :-)
This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch.
drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb;
unsigned int *hcd;
unsigned int hcd;I think this should be a pointer. Perhaps a (struct ehci_hccr *?
Isn't it better to maintain it as unsigned int ? Since later when fdtdec_get_addr() is used, which returns u32 or u64, we can easily check against FDT_ADDR_T_NONE. ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed.
How about having a local variable in your decode routine like:
fdt_addr_t addr;
Yeah, seem a nice idea.
addr = fdtdec_get_addr()... if (addr == FDT_ADDR_NONE) { debug(...) return -1; } exynos->hcd = (struct ... *)addr;
I think this is better than holding a value that is the wrong type, and casting it throughout your driver. Best to get the casting/checking done at init time,
True, will modify this and post it.
};
+static struct exynos_ehci exynos;
static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */
exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg");if (exynos->hcd == NULL) {
exynos->hcd = fdtdec_get_addr(blob, node, "reg");if (exynos->hcd < 0) {You need to check for FDT_ADDR_NONE here.
Sure.
debug("Can't get the EHCI register address\n"); return -ENXIO; }@@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}
struct exynos_ehci *ctx = &exynos;
exynos_usb_parse_dt(gd->fdt_blob, exynos);
exynos_usb_parse_dt(gd->fdt_blob, ctx);
setup_usb_phy(exynos->usb);
setup_usb_phy(ctx->usb);
*hccr = (struct ehci_hccr *)(exynos->hcd);
*hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));@@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
kfree(exynos);return 0;}
@@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) {
struct exynos_ehci *exynos = NULL;exynos = (struct exynos_ehci *)kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL);if (!exynos) {debug("failed to allocate exynos ehci context\n");return -ENOMEM;}exynos_usb_parse_dt(gd->fdt_blob, exynos);reset_usb_phy(exynos->usb);
struct exynos_ehci *ctx = &exynos;
kfree(exynos);
reset_usb_phy(ctx->usb); return 0;}
1.7.6.5
--
Enabling the non-dt path for the driver so that we don't get any build errors for non-dt configuration.
Signed-off-by: Vivek Gautam gautam.vivek@samsung.com --- drivers/usb/host/ehci-exynos.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 68f12fc..8c75c07 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -47,6 +47,7 @@ struct exynos_ehci {
static struct exynos_ehci exynos;
+#ifdef CONFIG_OF_CONTROL static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -87,6 +88,7 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos)
return 0; } +#endif
/* Setup the EHCI host controller. */ static void setup_usb_phy(struct exynos_usb_phy *usb) @@ -148,7 +150,12 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) { struct exynos_ehci *ctx = &exynos;
+#ifdef CONFIG_OF_CONTROL exynos_usb_parse_dt(gd->fdt_blob, ctx); +#else + ctx->usb = (struct exynos_usb_phy *)samsung_get_base_usb_phy(); + ctx->hcd = samsung_get_base_usb_ehci(); +#endif
setup_usb_phy(ctx->usb);
participants (3)
-
Simon Glass -
Vivek Gautam -
Vivek Gautam