linux-kernel - RE: [PATCH 3/3] usb: cdns3: Enable workaround for USB2.0 PHY Rx compliance test PHY lockup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 26 Aug 2020 08:07:05 +0000
From:   Peter Chen <peter.chen@....com>
To:     Pawel Laszczak <pawell@...ence.com>
CC:     Roger Quadros <rogerq@...com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        Rahul Kumar <kurahul@...ence.com>,
        "nsekhar@...com" <nsekhar@...com>,
        "vigneshr@...com" <vigneshr@...com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH 3/3] usb: cdns3: Enable workaround for USB2.0 PHY Rx
 compliance test PHY lockup

 
> 
> 
> >
> >On 20-08-26 04:04:01, Pawel Laszczak wrote:
> >> >On 20-08-25 15:00:59, Roger Quadros wrote:
> >> >> From: Pawel Laszczak <pawell@...ence.com>
> >> >>
> >> >> USB2.0 PHY hangs in Rx Compliance test when the incoming packet
> >> >> amplitude is varied below and above the Squelch Level of Receiver
> >> >> during the active packet multiple times.
> >> >>
> >> >> Version 1 of the controller allows PHY to be reset when RX fail
> >> >> condition is detected to work around the above issue. This feature
> >> >> is disabled by default and needs to be enabled using a bit from
> >> >> the newly added PHYRST_CFG register. This patch enables the workaround.
> >> >>
> >> >> As there is no way to distinguish between the controller version
> >> >> before the device controller is started we need to rely on a DT
> >> >> property to decide when to apply the workaround.
> >> >
> >> >Pawel, it could know the controller version at cdns3_gadget_start,
> >> >but the controller starts when it tries to bind gadget driver, at
> >> >that time, it has already known the controller version.
> >> >
> >> >For me, the device controller starts is using USB_CONF.DEVEN (Device
> >> >Enable) through usb_gadget_connect, I am not sure if it is the same
> >> >with yours.
> >> >
> >>
> >> Yes in device mode driver knows controller version but this
> >> workaround Must be enabled also in host mode. In host mode the
> >> controller doesn't have access to device registers. The controller
> >> version is placed in device register.
> >>
> >
> >You may suggest your design team adding CHIP_VER register at global
> >register region, it will easy the software engineer life.
> >
> >From what I read, this register is only enabling USB2 PHY reset
> >software control, it needs for all chips with rev 0x0002450D, and the
> >place you current change is only for 0x0002450D, right?
> 
> Even I could say that this workaround should be enabled only for Specific USB2
> PHY  (only 0x0002450D)
> 
> This bit should not have any impact for Cadence PHY but it can has Impact for third
> party PHYs.
> 

So, it is related to specific PHY, but enable this specific PHY reset bit is at controller region, why don't
put this enable bit at PHY region?

So, you use controller's device property to know this specific PHY, can controller know this specific
PHY dynamically?

Peter

> Pawel
> 
> >
> >Peter
> >
> >> Pawel
> >>
> >> >Peter
> >> >
> >> >
> >> >>
> >> >> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
> >> >> Signed-off-by: Roger Quadros <rogerq@...com>
> >> >> ---
> >> >>  drivers/usb/cdns3/core.c |  2 ++
> >> >>  drivers/usb/cdns3/core.h |  1 +
> >> >>  drivers/usb/cdns3/drd.c  | 12 ++++++++++++
> >> >> drivers/usb/cdns3/drd.h  |  5 ++++-
> >> >>  4 files changed, 19 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> >> index 5c1586ec7824..34b36487682b 100644
> >> >> --- a/drivers/usb/cdns3/core.c
> >> >> +++ b/drivers/usb/cdns3/core.c
> >> >> @@ -443,6 +443,8 @@ static int cdns3_probe(struct platform_device *pdev)
> >> >>  		return -ENXIO;
> >> >>  	}
> >> >>
> >> >> +	cdns->phyrst_a_enable = device_property_read_bool(dev,
> >> >> +"cdns,phyrst-a-enable");
> >> >> +
> >> >>  	cdns->otg_res = *res;
> >> >>
> >> >>  	mutex_init(&cdns->mutex);
> >> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> >> >> index 1ad1f1fe61e9..24cf0f1b5726 100644
> >> >> --- a/drivers/usb/cdns3/core.h
> >> >> +++ b/drivers/usb/cdns3/core.h
> >> >> @@ -76,6 +76,7 @@ struct cdns3 {
> >> >>  #define CDNS3_CONTROLLER_V0	0
> >> >>  #define CDNS3_CONTROLLER_V1	1
> >> >>  	u32				version;
> >> >> +	bool				phyrst_a_enable;
> >> >>
> >> >>  	int				otg_irq;
> >> >>  	int				dev_irq;
> >> >> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> >> >> index 6234bcd6158a..b74803e9703d 100644
> >> >> --- a/drivers/usb/cdns3/drd.c
> >> >> +++ b/drivers/usb/cdns3/drd.c
> >> >> @@ -42,6 +42,18 @@ int cdns3_set_mode(struct cdns3 *cdns, enum
> usb_dr_mode mode)
> >> >>  			reg = readl(&cdns->otg_v1_regs->override);
> >> >>  			reg |= OVERRIDE_IDPULLUP;
> >> >>  			writel(reg, &cdns->otg_v1_regs->override);
> >> >> +
> >> >> +			/*
> >> >> +			 * Enable work around feature built into the
> >> >> +			 * controller to address issue with RX Sensitivity
> >> >> +			 * est (EL_17) for USB2 PHY. The issue only occures
> >> >> +			 * for 0x0002450D controller version.
> >> >> +			 */
> >> >> +			if (cdns->phyrst_a_enable) {
> >> >> +				reg = readl(&cdns->otg_v1_regs->phyrst_cfg);
> >> >> +				reg |= PHYRST_CFG_PHYRST_A_ENABLE;
> >> >> +				writel(reg, &cdns->otg_v1_regs->phyrst_cfg);
> >> >> +			}
> >> >>  		} else {
> >> >>  			reg = readl(&cdns->otg_v0_regs->ctrl1);
> >> >>  			reg |= OVERRIDE_IDPULLUP_V0;
> >> >> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> >> >> index 7e7cf7fa2dd3..f1ccae285a16 100644
> >> >> --- a/drivers/usb/cdns3/drd.h
> >> >> +++ b/drivers/usb/cdns3/drd.h
> >> >> @@ -31,7 +31,7 @@ struct cdns3_otg_regs {
> >> >>  	__le32 simulate;
> >> >>  	__le32 override;
> >> >>  	__le32 susp_ctrl;
> >> >> -	__le32 reserved4;
> >> >> +	__le32 phyrst_cfg;
> >> >>  	__le32 anasts;
> >> >>  	__le32 adp_ramp_time;
> >> >>  	__le32 ctrl1;
> >> >> @@ -153,6 +153,9 @@ struct cdns3_otg_common_regs {
> >> >>  /* Only for CDNS3_CONTROLLER_V0 version */
> >> >>  #define OVERRIDE_IDPULLUP_V0		BIT(24)
> >> >>
> >> >> +/* PHYRST_CFG - bitmasks */
> >> >> +#define PHYRST_CFG_PHYRST_A_ENABLE     BIT(0)
> >> >> +
> >> >>  #define CDNS3_ID_PERIPHERAL		1
> >> >>  #define CDNS3_ID_HOST			0
> >> >>
> >> >> --
> >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >> >>
> >> >
> >> >--
> >> >
> >> >Thanks,
> >> >Peter Chen
> >
> >--
> >
> >Thanks,
> >Peter Chen