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 ?
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
