From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Fri, 16 Sep 2016 11:10:53 +0200 Subject: [U-Boot] [PATCH v3] drivers: usb: xhci-fsl: Implement Erratum A-010151 for FSL USB3 controller In-Reply-To: References: <1473830127-31653-1-git-send-email-sriram.dash@nxp.com> <8c69821e-8678-a3ca-041d-68ff7ebbf241@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 >>>>> Signed-off-by: Rajesh Bhagat >>>>> --- >>>>> 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. > 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 ? 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() ? >>>>> +} >>>>> 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. >>> 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