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

[Vivek Gautam]

Hi,


On Wed, Mar 6, 2013 at 7:27 AM, Simon Glass <sjg at chromium.org> wrote:
> On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam <gautamvivek1987 at gmail.com> wrote:
>> 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.
>
> 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