[U-Boot] [PATCH v3] drivers: usb: xhci-fsl: Implement Erratum A-010151 for FSL USB3 controller

[Sriram Dash]

>From: Marek Vasut [mailto:marex at denx.de]
>On 09/16/2016 07:50 AM, Sriram Dash wrote:
>>> From: Marek Vasut [mailto:marex at denx.de] On 09/15/2016 08:31 AM,
>>> Sriram Dash wrote:
>>>>> From: Marek Vasut [mailto:marex at denx.de] On 09/14/2016 07:15 AM,
>>>>> Sriram Dash wrote:
>>>>>> Currently the controller by default enables the Receive Detect
>>>>>> feature in P3 mode in USB 3.0 PHY. However, USB 3.0 PHY does not
>>>>>> reliably support receive detection in P3 mode.
>>>>>> Enabling the USB3 controller to configure USB in P2 mode whenever
>>>>>> the Receive Detect feature is required.
>>>>>>
>>>>>> Signed-off-by: Sriram Dash <sriram.dash at nxp.com>
>>>>>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at nxp.com>
>>>>>> ---
>>>>>> Changes in v3:
>>>>>>   - Fixing conflicts and repost
>>>>>
>>>>> Changelog of this verbosity is completely useless.
>>>>>
>>>>
>>>> I will take care the next time.
>>>>
>>>>>> Changes in v2:
>>>>>>   - Do Soc ver checking for applying erratum
>>>>>>
>>>>>>  drivers/usb/common/fsl-errata.c | 26 ++++++++++++++++++++++++++
>>>>>>  drivers/usb/host/xhci-dwc3.c    |  5 +++++
>>>>>>  drivers/usb/host/xhci-fsl.c     |  8 ++++++++
>>>>>>  include/fsl_usb.h               |  1 +
>>>>>>  include/linux/usb/dwc3.h        |  2 ++
>>>>>>  5 files changed, 42 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/common/fsl-errata.c
>>>>>> b/drivers/usb/common/fsl-errata.c index 183bf2b..f2bffba 100644
>>>>>> --- a/drivers/usb/common/fsl-errata.c
>>>>>> +++ b/drivers/usb/common/fsl-errata.c
>>>>>> @@ -190,4 +190,30 @@ bool has_erratum_a008751(void)
>>>>>>  	return false;
>>>>>>  }
>>>>>>
>>>>>> +bool has_erratum_a010151(void)
>>>>>> +{
>>>>>> +	u32 svr = get_svr();
>>>>>> +	u32 soc = SVR_SOC_VER(svr);
>>>>>> +
>>>>>> +	switch (soc) {
>>>>>> +#ifdef CONFIG_ARM64
>>>>>> +	case SVR_LS2080A:
>>>>>> +	case SVR_LS2085A:
>>>>>> +	case SVR_LS1046A:
>>>>>> +	case SVR_LS1012A:
>>>>>> +		return IS_SVR_REV(svr, 1, 0);
>>>>>> +	case SVR_LS1043A:
>>>>>> +		return IS_SVR_REV(svr, 1, 0) || IS_SVR_REV(svr, 1, 1); #endif
>>>>>> +#ifdef CONFIG_LS102XA
>>>>>> +	case SOC_VER_LS1020:
>>>>>> +	case SOC_VER_LS1021:
>>>>>> +	case SOC_VER_LS1022:
>>>>>> +	case SOC_VER_SLS1020:
>>>>>> +		return IS_SVR_REV(svr, 2, 0);
>>>>>> +#endif
>>>>>> +	}
>>>>>> +	return false;
>>>>>> +}
>>>>>> +
>>>>>>  #endif
>>>>>> diff --git a/drivers/usb/host/xhci-dwc3.c
>>>>>> b/drivers/usb/host/xhci-dwc3.c index 33961cd..adbd9b5 100644
>>>>>> --- a/drivers/usb/host/xhci-dwc3.c
>>>>>> +++ b/drivers/usb/host/xhci-dwc3.c
>>>>>> @@ -97,3 +97,8 @@ void dwc3_set_fladj(struct dwc3 *dwc3_reg, u32 val)
>>>>>>  	setbits_le32(&dwc3_reg->g_fladj, GFLADJ_30MHZ_REG_SEL |
>>>>>>  			GFLADJ_30MHZ(val));
>>>>>>  }
>>>>>> +
>>>>>> +void dwc3_set_rxdetect_power_mode(struct dwc3 *dwc3_reg, u32 val) {
>>>>>> +	setbits_le32(&dwc3_reg->g_usb3pipectl[0], val);
>>>>>
>>>>> So what would happen if I wanted to clean some bits instead ?
>>>>
>>>> Setting the Rx detect in P2 mode is a one time job, and it does not
>>>> change. Hence, IMO, the clear bit functionality is not required.
>>>
>>> Until an SoC comes which has some bits set there and someone wants to
>>> clear them . At which point, this code will serve as a trap .
>>>
>>
>> The default value of the register bit g_usb3pipectl is 0.
>
>For this particular hardware revision. That can be changed in some later revision
>and thus any user will fall into this trap.
>

Ok. I get it now. Anyways, I will be taking it up in the fsl file and removing
the function.

>> And the default value is
>> not modified anytime during execution. For the unreliable rxdetect, it
>> is set to P2 mode(by setting the bit to 1). So, if any Soc chooses the
>> rxdetect as P3, they will not use the function
>> dwc3_set_rxdetect_power_mode to set the bit.
>
>Bit or bits ? If you are setting exactly one bit, why does the function have u32
>argument at all ?
>

We are modifying single bit. Now that we are planning to have it done with
single command, as mentioned below, the function will be dropped.

>And you still didn't explain why is the setbits_le32() here and not plain writel() .
>
>>>>> Why do you even need a dedicated function to write a single register?
>>>>>
>>>>
>>>> The dwc3 phy for the specific controller version
>>>
>>> This should be explicitly documented with a comment here.
>>>
>>
>> OK. Will take care in the next patch v4.
>>
>>>> does not reliably
>>>> support Rx Detect in P3 mode(P3 is the default setting). So, this
>>>> setting can be used by any other Soc, apart from freescale. IMO,
>>>> this function is required.
>>>
>>> So why won't such a system call single register write directly ?
>>>
>>
>> I agree to your point. We can set the bit from fsl specific file with
>> the function setbits_le32(fsl_xhci->dwc3_reg->g_usb3pipectl[0],
>> DWC3_GUSB3PIPECTL_DISRXDETP3);
>> If any other Soc, other than fsl/nxp wants the functionality, they
>> will be using the same in their respective code. What do you say?
>
>Why do you use setbits_le32() instead of writel() ?
>

We will be modifying a single bit. So, better to use setbit_le32 and
leave other bits unchanged.


>>>>>> +}
>>>>>> diff --git a/drivers/usb/host/xhci-fsl.c
>>>>>> b/drivers/usb/host/xhci-fsl.c index 0e3e056..9297ced 100644
>>>>>> --- a/drivers/usb/host/xhci-fsl.c
>>>>>> +++ b/drivers/usb/host/xhci-fsl.c
>>>>>> @@ -84,6 +84,14 @@ static int fsl_xhci_core_init(struct fsl_xhci *fsl_xhci)
>>>>>>  	/* Change beat burst and outstanding pipelined transfers requests */
>>>>>>  	fsl_xhci_set_beat_burst_length(fsl_xhci->dwc3_reg);
>>>>>>
>>>>>> +	/*
>>>>>> +	 * A-010151: USB controller to configure USB in P2 mode
>>>>>> +	 * whenever the Receive Detect feature is required
>>>>>> +	 */
>>>>>> +	if (has_erratum_a010151())
>>>>>> +		dwc3_set_rxdetect_power_mode(fsl_xhci->dwc3_reg,
>>>>>> +					     DWC3_GUSB3PIPECTL_DISRXDETP3);
>>>>>
>>>>> Can't you parse these errata infos from device tree ?
>>>>>
>>>>
>>>> Currently, all the freescale Socs using this controller are not using DM.
>>>
>>> I am asking about device tree, not driver model. These two are orthogonal.
>>>
>>
>> Sorry. My bad. But all the socs are not using the device tree now for uboot.
>> I am planning to modify the implementation when the device tree
>> support is used by all the socs using xhci controller for fsl/nxp. What is your
>opinion?
>
>That is fine.
>

OK.

>>>> Shall we proceed with this solution currently, and then when the DM
>>>> is supported by all Socs, modify the implementation according to device tree?
>>>> What do you suggest?
>>>>
>>> [...]
>>>
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>
>
>--
>Best regards,
>Marek Vasut