Hi,
On Wed, Mar 6, 2013 at 7:27 AM, Simon Glass <[email protected]> wrote: > On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam <[email protected]> > wrote: >> Hi, >> >> >> On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass <[email protected]> wrote: >>> Hi, >>> >>> On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <[email protected]> >>> 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 <[email protected]> >>> >>> 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 >>>> >>> >> >> -- -- Thanks & Regards Vivek _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
