[U-Boot] [PATCH v2 2/5] usb: xhci: fsl: code cleanup for device tree fixup for fsl usb controllers

[Sriram Dash]

>-----Original Message-----
>From: Marek Vasut [mailto:marex at denx.de]
>Sent: Thursday, June 02, 2016 6:25 PM
>To: Sriram Dash <sriram.dash at nxp.com>; u-boot at lists.denx.de
>Cc: york sun <york.sun at nxp.com>; albert.u.boot at aribaud.net; Rajesh Bhagat
><rajesh.bhagat at nxp.com>
>Subject: Re: [PATCH v2 2/5] usb: xhci: fsl: code cleanup for device tree fixup for fsl
>usb controllers
>
>On 06/02/2016 08:54 AM, Sriram Dash wrote:
>> Performs code cleanup for device tree fixup for fsl usb controllers by
>> making functions to handle these similar errata checking code.
>>
>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at nxp.com>
>> Signed-off-by: Sriram Dash <sriram.dash at nxp.com>
>> ---
>> Changes in v2:
>>   - added patch description
>>   - remove the MACRO and use fdt_fixup_erratum function instead
>>
>>
>>  drivers/usb/common/fsl-dt-fixup.c | 58
>> +++++++++++++++++----------------------
>>  1 file changed, 25 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/usb/common/fsl-dt-fixup.c
>> b/drivers/usb/common/fsl-dt-fixup.c
>> index 6f31932..cbb008b 100644
>> --- a/drivers/usb/common/fsl-dt-fixup.c
>> +++ b/drivers/usb/common/fsl-dt-fixup.c
>> @@ -99,6 +99,23 @@ static int fdt_fixup_usb_erratum(void *blob, const char
>*prop_erratum,
>>  	return node_offset;
>>  }
>>
>> +void fdt_fixup_erratum(int *usb_erratum_off, void *blob,
>> +		       char *str, bool (*has_erratum)(void)) {
>> +	char buf[32] = {0};
>> +
>> +	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
>> +	if (has_erratum()) {
>
>Invert the condition here so you won't have the indentation problems below:
>
>if (!(has_erratum())
>	return;
>... other stuff.
>

Thank you for the suggestion. Looks better in a single line. Will change in V3.

>
>> +		*usb_erratum_off =  fdt_fixup_usb_erratum
>> +				   (blob,
>> +				    buf,
>> +				    *usb_erratum_off);
>> +		if (*usb_erratum_off < 0)
>> +			return;
>
>If fdt_fixup_usb_erratum() fails, this function will return, but the code below will
>continue. This changes the logic of the code to the worse, so fix this.
>

Yes. You are correct. I will now return ENOSPC in case of failure and check that in
fdt_fixup_dr_usb function to maintain the same logic as before.

>> +		debug("Adding USB erratum %s\n", str);
>> +	}
>> +}
>> +
>>  void fdt_fixup_dr_usb(void *blob, bd_t *bd)  {
>>  	static const char * const modes[] = { "host", "peripheral", "otg" };
>> @@ -164,39 +181,14 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
>>  		if (usb_phy_off < 0)
>>  			return;
>>
>> -		if (has_erratum_a006261()) {
>> -			usb_erratum_a006261_off =  fdt_fixup_usb_erratum
>> -						   (blob,
>> -						    "fsl,usb-erratum-a006261",
>> -						    usb_erratum_a006261_off);
>> -			if (usb_erratum_a006261_off < 0)
>> -				return;
>> -		}
>> -
>> -		if (has_erratum_a007075()) {
>> -			usb_erratum_a007075_off =  fdt_fixup_usb_erratum
>> -						   (blob,
>> -						    "fsl,usb-erratum-a007075",
>> -						    usb_erratum_a007075_off);
>> -			if (usb_erratum_a007075_off < 0)
>> -				return;
>> -		}
>> +		fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
>> +				  "a006261", has_erratum_a006261);
>> +		fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
>> +				  "a007075", has_erratum_a007075);
>> +		fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
>> +				  "a007792", has_erratum_a007792);
>> +		fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
>> +				  "a005697", has_erratum_a005697);
>
>Are these usb_erratum_aNNNNNN_off variables needed at all ? Why are they even
>passed into the function ? I think they can be local to the function.
>

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.
Now, we cannot locally deal with the variable, reason being, in function fdt_fixup_erratum, To make it keep track of the offset, I have again generate multiple variables for that many erratas.
Also, I have to make each variable static, in order to keep track of the controller specific errata Applicability.

>> -		if (has_erratum_a007792()) {
>> -			usb_erratum_a007792_off =  fdt_fixup_usb_erratum
>> -						   (blob,
>> -						    "fsl,usb-erratum-a007792",
>> -						    usb_erratum_a007792_off);
>> -			if (usb_erratum_a007792_off < 0)
>> -				return;
>> -		}
>> -		if (has_erratum_a005697()) {
>> -			usb_erratum_a005697_off =  fdt_fixup_usb_erratum
>> -						   (blob,
>> -						    "fsl,usb-erratum-a005697",
>> -						    usb_erratum_a005697_off);
>> -			if (usb_erratum_a005697_off < 0)
>> -				return;
>> -		}
>>  	}
>>  }
>>
>
>
>--
>Best regards,
>Marek Vasut