[U-Boot] [PATCH v2 5/5] usb: xhci: fsl: Add workaround for USB erratum A-008751

[Marek Vasut <marex@denx.de>]

On 06/06/2016 06:24 AM, Sriram Dash wrote:
>> -----Original Message-----
>> From: Marek Vasut [mailto:marex at denx.de]
>> Sent: Thursday, June 02, 2016 6:31 PM
>> To: Sriram Dash <sriram.dash@nxp.com>; u-boot at lists.denx.de
>> Cc: york sun <york.sun@nxp.com>; albert.u.boot at aribaud.net; Rajesh Bhagat
>> <rajesh.bhagat@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.dash@nxp.com>
>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@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