>-----Original Message-----
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Monday, June 06, 2016 6:15 PM
>To: Sriram Dash <sriram.d...@nxp.com>; u-boot@lists.denx.de
>Cc: york sun <york....@nxp.com>; albert.u.b...@aribaud.net; Rajesh Bhagat
><rajesh.bha...@nxp.com>
>Subject: Re: [PATCH v2 5/5] usb: xhci: fsl: Add workaround for USB erratum A-
>008751
>
>On 06/06/2016 06:24 AM, Sriram Dash wrote:
>>> -----Original Message-----
>>> From: Marek Vasut [mailto:ma...@denx.de]
>>> Sent: Thursday, June 02, 2016 6:31 PM
>>> To: Sriram Dash <sriram.d...@nxp.com>; u-boot@lists.denx.de
>>> Cc: york sun <york....@nxp.com>; albert.u.b...@aribaud.net; Rajesh
>>> Bhagat <rajesh.bha...@nxp.com>
>>> Subject: Re: [PATCH v2 5/5] usb: xhci: fsl: Add workaround for USB
>>> erratum A-
>>> 008751
>>>
>>> On 06/02/2016 08:54 AM, Sriram Dash wrote:
>>>> This patch is doing the following:
>>>> 1. Implementing the errata for LS2080.
>>>> 2. Adding fixup for fdt for LS2080.
>>>>
>>>> Signed-off-by: Sriram Dash <sriram.d...@nxp.com>
>>>> Signed-off-by: Rajesh Bhagat <rajesh.bha...@nxp.com>
>>>> ---
>>>> Changes in v2:
>>>>   - Reworked for changes done in errata checking code.
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>>>> b/drivers/usb/common/fsl-dt-fixup.c
>>>> index 7c039cb..fd85439 100644
>>>> --- a/drivers/usb/common/fsl-dt-fixup.c
>>>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>>>> @@ -125,6 +125,7 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
>>>>    int usb_erratum_a007075_off = -1;
>>>>    int usb_erratum_a007792_off = -1;
>>>>    int usb_erratum_a005697_off = -1;
>>>> +  int usb_erratum_a008751_off = -1;
>>>
>>> Will there be an ever-growing list of unused variables here ?
>>>
>>
>> Same as in Patch 2/5 code cleanup for device tree fixup for fsl usb 
>> controllers
>usb_erratum_aNNNNNN_off variable are keeping track of the device tree fixup for
>errata NNNNNN. The code checks errata applicability for each controller and 
>tries
>to fix the device tree accordingly. During this time, the variable
>usb_erratum_aNNNNNN_off is updated to the offset of device tree, if the device
>tree is fixed. Now, for the second controller, when it tries to fix the device 
>tree, it
>checks from the same offset obtained. As the API for fdt_setprop is such that 
>the
>fixup will occur as soon as the API finds first match, if this variable
>usb_erratum_aNNNNNN_off is not maintained, the errata will be fixed always for
>the first usb controller it comes across the device tree. So, we need this 
>variable.
>
>What will happen if you have two different controllers in the system and each 
>of
>them has different set of erratas ? Will this code fail at handling them by 
>applying
>wrong sets of erratas ?
>

Yes, you are right.
For the different controllers with different erratas, I am planning
to handle it by passing the controller type(ex: fsl-usb2-dr) from the
fdt_fixup_erratum function, for which , the errata would be applied,
and decided in  fdt_fixup_usb_erratum function in V3.

>btw Can you please fix your mailer to break lines at 80 chars ?
>
>>>>    int usb_mode_off = -1;
>>>>    int usb_phy_off = -1;
>>>>    char str[5];
>>>> @@ -190,6 +191,8 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
>>>>                              "a007792", has_erratum_a007792);
>>>>            fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
>>>>                              "a005697", has_erratum_a005697);
>>>> +          fdt_fixup_erratum(&usb_erratum_a008751_off, blob,
>>>> +                            "a008751", has_erratum_a008751);
>>>>
>>>>    }
>>>>  }
>>>> diff --git a/drivers/usb/common/fsl-errata.c
>>>> b/drivers/usb/common/fsl-errata.c index 95918fc..ebe60a8 100644
>>>> --- a/drivers/usb/common/fsl-errata.c
>>>> +++ b/drivers/usb/common/fsl-errata.c
>>>> @@ -175,4 +175,19 @@ bool has_erratum_a004477(void)
>>>>    return false;
>>>>  }
>>>>
>>>> +bool has_erratum_a008751(void)
>>>> +{
>>>> +  u32 svr = get_svr();
>>>> +  u32 soc = SVR_SOC_VER(svr);
>>>> +
>>>> +  switch (soc) {
>>>> +#ifdef CONFIG_ARM64
>>>> +  case SVR_LS2080:
>>>> +  case SVR_LS2085:
>>>> +          return IS_SVR_REV(svr, 1, 0);
>>>> +#endif
>>>> +  }
>>>> +  return false;
>>>> +}
>>>> +
>>>>  #endif
>>>> diff --git a/drivers/usb/host/xhci-fsl.c
>>>> b/drivers/usb/host/xhci-fsl.c index 05f09d7..d55ed87 100644
>>>> --- a/drivers/usb/host/xhci-fsl.c
>>>> +++ b/drivers/usb/host/xhci-fsl.c
>>>> @@ -15,6 +15,8 @@
>>>>  #include <linux/usb/xhci-fsl.h>
>>>>  #include <linux/usb/dwc3.h>
>>>>  #include "xhci.h"
>>>> +#include <fsl_errata.h>
>>>> +#include <fsl_usb.h>
>>>>
>>>>  /* Declare global data pointer */
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>> @@ -27,6 +29,31 @@ __weak int __board_usb_init(int index, enum
>>>> usb_init_type
>>> init)
>>>>    return 0;
>>>>  }
>>>>
>>>> +static inline bool  erratum_a008751(void)
>>>
>>> Drop the inline, let compiler decide. Also, make this an int instead
>>> of bool and return 0 on success, 1 on error.
>>>
>>
>> Ok. Will drop inline and make static int from static bool.
>> Return 0 on success instead of true.
>> Return 1 on error instead of false.
>>
>>>> +{
>>>> +#if defined(CONFIG_TARGET_LS2080AQDS) ||
>>> defined(CONFIG_TARGET_LS2080ARDB)
>>>> +  u32 __iomem *scfg = (u32 __iomem *)SCFG_BASE;
>>>> +  writel(SCFG_USB3PRM1CR_INIT, scfg + SCFG_USB3PRM1CR / 4);
>>>> +  return true;
>>>> +#endif
>>>> +  return false;
>>>> +}
>>>> +
>>>> +#define APPLY_ERRATUM(id)                                         \
>>>> +do {                                                                      
>>>> \
>>>> +  bool ret;                                                       \
>>>> +  if (has_erratum_##id()) {                                       \
>>>> +          ret = erratum_##id();                                   \
>>>> +          if (ret <= 0)                                           \
>>>> +                  puts("Failed to apply erratum " #id "\n");      \
>>>> +  }                                                               \
>>>> +} while (0)
>>>
>>> Turn this into a function.
>>>
>>
>> On a second thought, I guess I will drop the MACRO and directly call
>> the
>> has_erratum_a008751() etc from fsl_apply_xhci_errata, instead of going for a
>function.
>> What do you say?
>
>Fine
>
>>>> +static void fsl_apply_xhci_errata(void) {
>>>> +  APPLY_ERRATUM(a008751);
>>>> +}
>>>> +
>>>>  static int fsl_xhci_core_init(struct fsl_xhci *fsl_xhci)  {
>>>>    int ret = 0;
>>>> @@ -69,6 +96,8 @@ int xhci_hcd_init(int index, struct xhci_hccr
>>>> **hccr, struct
>>> xhci_hcor **hcor)
>>>>            return ret;
>>>>    }
>>>>
>>>> +  fsl_apply_xhci_errata();
>>>> +
>>>>    ret = fsl_xhci_core_init(ctx);
>>>>    if (ret < 0) {
>>>>            puts("Failed to initialize xhci\n"); diff --git
>>>> a/include/fsl_usb.h b/include/fsl_usb.h index d183349..fc72fb9
>>>> 100644
>>>> --- a/include/fsl_usb.h
>>>> +++ b/include/fsl_usb.h
>>>> @@ -94,5 +94,6 @@ bool has_erratum_a007798(void);  bool
>>>> has_erratum_a007792(void);  bool has_erratum_a005697(void);  bool
>>>> has_erratum_a004477(void);
>>>> +bool has_erratum_a008751(void);
>>>>  #endif
>>>>  #endif /*_ASM_FSL_USB_H_ */
>>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>
>
>--
>Best regards,
>Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to