[U-Boot] [PATCH v2 1/2] usb: ehci: exynos: Fix multiple FDT decode

[Vivek Gautam]

Hi,


On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <gautam.vivek at 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 at 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
>>
>

-- 
Thanks & Regards
Vivek