Re: [regression] USB power management failure to suspend / high CPU usage
On 03.02.2019 04:08, Eric Blau wrote: I finally was able to complete the bisect on this issue. It seems that this commit introduced the regression on my MacBook Pro: cc8b329fef53c74a4abf98b0755b3832d572d6ce is the first bad commit commit cc8b329fef53c74a4abf98b0755b3832d572d6ce Author: Mathias Nyman Date: Thu Nov 15 11:38:41 2018 +0200 usb: xhci: Prevent bus suspend if a port connect change or polling state is detected commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. There's an additional fix for that patch, in 4.19 stable: commit e13bfb357f5bc04f9e7ccff7d07770388062a8cc Author: Mathias Nyman Date: Fri Dec 14 10:54:43 2018 +0200 xhci: Don't prevent USB2 bus suspend in state check intended for USB3 only commit 45f750c16cae3625014c14c77bd9005eda975d35 upstream. If you already have that fix then one of the USB ports may be in a odd state when suspending Does adding dynamic debug for xhci_hub_control show any messages when suspending? mount -t debugfs none /sys/kernel/debug echo -n 'func xhci_bus_suspend =p' > /sys/kernel/debug/dynamic_debug/control -Mathias
Re: [PATCH] usb: dwc2: Reset device address on EnumDone
Hi Felipe, On 1/21/2019 11:13 AM, Minas Harutyunyan wrote: > Hi Felipe, > > On 12/12/2018 3:43 PM, Minas Harutyunyan wrote: >> Initially resetting device address was done in USB RESET interrupt >> handler. In case, when power saving mode enabled (hibernation) USB >> RESET interrupt handled in dwc2_handle_gpwrdn_intr() and then it >> not seen in dwc2_hsotg_irq() handler. This is why reset device >> address to zero moved from USB RESET handler to EnumDone handler. >> >> Signed-off-by: Minas Harutyunyan >> --- >>drivers/usb/dwc2/gadget.c | 6 +++--- >>1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 68ad75a7460d..7f922f19f8e1 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -3072,6 +3072,9 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg >> *hsotg) >> >> dev_dbg(hsotg->dev, "EnumDone (DSTS=0x%08x)\n", dsts); >> >> +/* Reset device address to zero */ >> +dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >> + >> /* >> * note, since we're limited by the size of transfer on EP0, and >> * it seems IN transfers must be a even number of packets we do >> @@ -3614,9 +3617,6 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) >> /* Report disconnection if it is not already done. */ >> dwc2_hsotg_disconnect(hsotg); >> >> -/* Reset device address to zero */ >> -dwc2_clear_bit(hsotg, DCFG, DCFG_DEVADDR_MASK); >> - >> if (usb_status & GOTGCTL_BSESVLD && connected) >> dwc2_hsotg_core_init_disconnected(hsotg, true); >> } >> > > This patch not seen yet in your testing/fixes or next. Any reason for > delay or you missed it? > > Thanks, > Minas > > Not seen yet. Ping again. Thanks, Minas
[PATCH] USB: serial: cp210x: add minimun baud rate for CP2105 SCI
From: Johanna Abrahamsson
This patch adds a minimum baud rate of 2400 for CP2105 SCI
According to the datasheet for CP2105, the SCI supports 2400 as the
lowest baud rate. As this is not heeded in the current code, an error
message 'failed set req 0x1e size 4 status: -32' when trying to set a
lower baud rate such as 300.
Since this is, as far as I can tell, the only cp210x chip with a minimum
baud rate higher than 300, I've added a special case to
cp210x_change_speed rather than adding a min_speed field in
cp210x_serial_private.
Signed-off-by: Johanna Abrahamsson
---
drivers/usb/serial/cp210x.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 336a3c0f9f2c..181abf7bb8c0 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -,9 +,12 @@ static void cp210x_change_speed(struct tty_struct *tty,
*/
if (priv->use_actual_rate)
baud = cp210x_get_actual_rate(serial, baud);
- else if (baud < 100)
+ else if (baud < 100) {
+ if (priv->partnum == CP210X_PARTNUM_CP2105 &&
+ cp210x_interface_num(serial) == 1)
+ baud = clamp(baud, 2400u, priv->max_speed);
baud = cp210x_get_an205_rate(baud);
- else if (baud > priv->max_speed)
+ } else if (baud > priv->max_speed)
baud = priv->max_speed;
dev_dbg(&port->dev, "%s - setting baud rate to %u\n", __func__, baud);
--
2.11.0
Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.
On Thu, Jan 31, 2019 at 11:52:29AM +, Pawel Laszczak wrote: > Patch moves some decoding functions from driver/usb/dwc3/debug.h driver > to driver/usb/common/debug.c file. These moved functions include: > dwc3_decode_get_status > dwc3_decode_set_clear_feature > dwc3_decode_set_address > dwc3_decode_get_set_descriptor > dwc3_decode_get_configuration > dwc3_decode_set_configuration > dwc3_decode_get_intf > dwc3_decode_set_intf > dwc3_decode_synch_frame > dwc3_decode_set_sel > dwc3_decode_set_isoch_delay > dwc3_decode_ctrl > > These functions are used also in inroduced cdns3 driver. > > All functions prefixes were changed from dwc3 to usb. Ick, why? > Also, function's parameters has been extended according to the name > of fields in standard SETUP packet. > Additionally, patch adds usb_decode_ctrl function to > include/linux/usb/ch9.h file. Why ch9.h? It's not something that is specified in the spec, it's a usb-specific thing :) Also, the api for that function is not ok. If you are going to make this something that the whole kernel can call, you have to fix it up: > +/** > + * usb_decode_ctrl - Returns human readable representation of control > request. > + * @str: buffer to return a human-readable representation of control request. > + * This buffer should have about 200 bytes. "about 200 bytes" is not very specific. Pass in the length so we know we don't overflow it. > + * @bRequestType: matches the USB bmRequestType field > + * @bRequest: matches the USB bRequest field > + * @wValue: matches the USB wValue field (CPU byte order) > + * @wIndex: matches the USB wIndex field (CPU byte order) > + * @wLength: matches the USB wLength field (CPU byte order) > + * > + * Function returns decoded, formatted and human-readable description of > + * control request packet. > + * > + * Important: wValue, wIndex, wLength parameters before invoking this > function > + * should be processed by le16_to_cpu macro. > + */ > +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest, > + __u16 wValue, __u16 wIndex, __u16 wLength); Why are you returning a value, isn't the data stored in str? Why not just return an int saying if the call succeeded or not? thanks, greg k-h
Re: [PATCH 0/8] Tegra XUSB gadget driver support
On Thu, Jan 03, 2019 at 03:34:51PM +0530, Nagarjuna Kristam wrote: > Add driver for XUSB device mode controller available on Tegra > Soc > > Patches 1-3 are phy driver changes to add support for device > mode Patches 4-7 are changes related to XUSB device mode > controller driver Patch 8 is to enable XUDC driver in defconfig > > Test Steps(USB 2.0): > - Enable "USB Gadget precomposed configurations" in defconfig > - Build, flash and boot Jetson TX1 > - Connect Jetson TX1 and Ubuntu device using USB A to Micro B > cable > - After boot on Jetson TX1 terminal usb0 network device should be > enumerated > - Assign static ip to usb0 on Jetson TX1 and corresponding net > device on ubuntu > - Run ping test and transfer test(used scp) to check data transfer > communication > > SS mode is verified by enabling Type A port as device > > Above patches are dependent on > https://patchwork.ozlabs.org/patch/976332/ > > Nagarjuna Kristam (8): > phy: tegra: xusb: t210: add XUSB device mode support > arm64: tegra: Add nvidia,usb3-port-fake entry to Jetson TX1 padctl > dt-bindings: phy: tegra-xusb-padctl: Add nvidia,usb3-port-fake entry Nit: it's usually best to send out the DT bindings changes before the driver or DT changes. That way reviewers can review in the natural order rather than find out after going through the implementation what the DT bindings intended. Thierry signature.asc Description: PGP signature
Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.
On Thu, Jan 31, 2019 at 11:52:29AM +, Pawel Laszczak wrote: > Patch moves some decoding functions from driver/usb/dwc3/debug.h driver > to driver/usb/common/debug.c file. These moved functions include: > dwc3_decode_get_status > dwc3_decode_set_clear_feature > dwc3_decode_set_address > dwc3_decode_get_set_descriptor > dwc3_decode_get_configuration > dwc3_decode_set_configuration > dwc3_decode_get_intf > dwc3_decode_set_intf > dwc3_decode_synch_frame > dwc3_decode_set_sel > dwc3_decode_set_isoch_delay > dwc3_decode_ctrl > > These functions are used also in inroduced cdns3 driver. /me hands Pawel a spell checker for the changelogs...
Re: [PATCH v3 4/6] usb:common Simplify usb_decode_get_set_descriptor function.
On Thu, Jan 31, 2019 at 11:52:31AM +, Pawel Laszczak wrote: > Patch moves switch responsible for decoding descriptor type > outside snprintf. It's little improves code readability. Should that last sentence read: "It improves code readability a little"? thanks, greg k-h
Re: [PATCH 3/8] dt-bindings: phy: tegra-xusb-padctl: Add nvidia,usb3-port-fake entry
On Thu, Jan 03, 2019 at 03:34:54PM +0530, Nagarjuna Kristam wrote: > Add binding details regarding nvidia,usb3-port-fake > > Signed-off-by: Nagarjuna Kristam > --- > Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt > b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt > index 3742c15..21a5541 100644 > --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt > +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt > @@ -158,6 +158,9 @@ Optional properties: >is internal. In the absence of this property the port is considered to be >external. > - vbus-supply: phandle to a regulator supplying the VBUS voltage. > +- nvidia,usb3-port-fake: A integer property whose presense informs that a > + fake USB3 port needs to be mapped for corresponding USB2 port. This entry > + is applicable only for Tegra210 based platforms which has USB2 only ports. You don't provide a rationale fo why this fake USB3 port needs to be specified. Doesn't the OTG port work without the fake USB3 port? Why does it need to be specified and which one should be used as fake? You mention elsewhere that an unused USB3 port is used on Jetson TX1, but what if there are no unused ports on a platform? Can we use any USB3 port for this purpose? Thierry signature.asc Description: PGP signature
Re: [PATCH v3 5/6] usb:cdns3 Add Cadence USB3 DRD Driver
On Thu, Jan 31, 2019 at 11:52:32AM +, Pawel Laszczak wrote: > This patch introduce new Cadence USBSS DRD driver > to linux kernel. > > The Cadence USBSS DRD Driver is a highly > configurable IP Core which can be > instantiated as Dual-Role Device (DRD), > Peripheral Only and Host Only (XHCI) > configurations. > > The current driver has been validated with > FPGA burned. We have support for PCIe > bus, which is used on FPGA prototyping. > > The host side of USBSS-DRD controller is compliance > with XHCI specification, so it works with > standard XHCI linux driver. Please line-wrap this properly at 72 columns, like your editor asks you to when you run git :) > > Signed-off-by: Pawel Laszczak > --- > drivers/usb/Kconfig|2 + > drivers/usb/Makefile |2 + > drivers/usb/cdns3/Kconfig | 44 + > drivers/usb/cdns3/Makefile | 14 + > drivers/usb/cdns3/cdns3-pci-wrap.c | 155 +++ > drivers/usb/cdns3/core.c | 403 ++ > drivers/usb/cdns3/core.h | 116 ++ > drivers/usb/cdns3/debug.h | 168 +++ > drivers/usb/cdns3/debugfs.c| 164 +++ > drivers/usb/cdns3/drd.c| 365 + > drivers/usb/cdns3/drd.h| 162 +++ > drivers/usb/cdns3/ep0.c| 907 + > drivers/usb/cdns3/gadget-export.h | 28 + > drivers/usb/cdns3/gadget.c | 1985 > drivers/usb/cdns3/gadget.h | 1207 + > drivers/usb/cdns3/host-export.h| 28 + > drivers/usb/cdns3/host.c | 72 + > drivers/usb/cdns3/trace.c | 23 + > drivers/usb/cdns3/trace.h | 404 ++ > 19 files changed, 6249 insertions(+) > create mode 100644 drivers/usb/cdns3/Kconfig > create mode 100644 drivers/usb/cdns3/Makefile > create mode 100644 drivers/usb/cdns3/cdns3-pci-wrap.c > create mode 100644 drivers/usb/cdns3/core.c > create mode 100644 drivers/usb/cdns3/core.h > create mode 100644 drivers/usb/cdns3/debug.h > create mode 100644 drivers/usb/cdns3/debugfs.c > create mode 100644 drivers/usb/cdns3/drd.c > create mode 100644 drivers/usb/cdns3/drd.h > create mode 100644 drivers/usb/cdns3/ep0.c > create mode 100644 drivers/usb/cdns3/gadget-export.h > create mode 100644 drivers/usb/cdns3/gadget.c > create mode 100644 drivers/usb/cdns3/gadget.h > create mode 100644 drivers/usb/cdns3/host-export.h > create mode 100644 drivers/usb/cdns3/host.c > create mode 100644 drivers/usb/cdns3/trace.c > create mode 100644 drivers/usb/cdns3/trace.h This is way too big to try to review all at once. You need to break this up into logical chunks, each one adding a single functionality to make it possible to actually review it. thanks, greg k-h
Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi, On Fri, Feb 01, 2019 at 10:02:19PM +, Michael Hsu wrote: > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a 1-based > index, not a 32-bit mode VDO) can cause incorrect matches if the > GET_ALTERNATE_MODES returns different ordering for recipient=connector and > recipient=sop for a particular svid. > > For example, UCSI command GET_ALTERNATE_MODES with recipient=connector returns > [0] SVID=0xff01, ModeVDO=0x0405 (Mode = 1) > [1] SVID=0xff01, ModeVDO=0x0805 (Mode = 2) > And UCSI command GET_ALTERNATE_MODES with recipient=sop returns > [0] SVID=0xff01, ModeVDO=0x0c05 (Mode = 1) > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM returns > index 1 (connector alternate mode = 1, i.e. SVID=0xff01, Mode=2, > ModeVDO=0x0805). > But, the function will be unable to match it with partner_alt_mode > corresponding to (SVID=0xff01,Mode=1). > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' bitwise > AND operator when masking the partner's mode VDO (0x0c05) against the > connector's mode VDO (0x405 or 0x805) to determine it there is an alternate > mode match. No top-posting! From now on inline your comments in your replies. The "process" guide in kernel should provide more information for you: https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists Your PPM is reporting a separate mode for every pin-assignment it supports. It really should _not_ do that! You need to be able to get the capabilities for DP alt mode with GET_ALTERNATE_MODE command in the format that the DP alt mode spec defines, also with the connector. You are not getting them like that. Now for example in both modes your connector can only act as DisplayPort sink (i.e. a display) which I'm pretty sure is not the case. With the current version of the DP alt mode spec we do not ever need more than one mode for DP. That mode needs to show all the supported pin assignments the device, partner or connector, supports. That is exactly how all the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 that I use for testing gives only one mode for the connector with SVID ff01. The mode VDO (or MID in UCSI lingo) has the value 0x1c46: SVID=0xff01, VDO=0x1c46 >From that you can clearly see that the connector can only act as the DisplayPort source, and that it support pin assignments C, D and E. So in practice you have a firmware bug. I understand why the PPM was made that way. That "hack" allows the PPM to workaround a limitation in UCSI where we in practice can't tell which configuration is in use at any given time. Unfortunately it can not be done like that. It leaves us with custom VDO values that we would have to be interpreted separately, that only your PPM supports. Please see if you can get the firmware (UCSI PPM) fixed. It really needs to expose only one mode for DisplayPort alt mode in the connector, and the VDO in it should be in the same form that it could be send to the partner, i.e. giving the actual DisplayPort capabilities for the connector. We should not do anything before you ask them to fix the PPM. Experience tells that if we workaround an issue in firmware, the firmware will never ever get fixed. And in this case the workaround may not be as simple as one would think. Even if we have to workaround this, it needs a separate patch with a good explanation. And it probable does not belong to this series. Thanks, -- heikki
Re: [PATCH 1/8] phy: tegra: xusb: t210: add XUSB device mode support
On Thu, Jan 03, 2019 at 03:34:52PM +0530, Nagarjuna Kristam wrote:
> Add support for XUSB device mode controller on Tegra210.
> Update PADCTL driver to set port cap based on DT config.
> Add code to handle property "nvidia,usb3-port-fake"
> Provide API's to control vbus override and utmi pad power control
You essentially list three features additions here, so it makes sense to
provide each of them in a separate patch. That helps reviewers
concentrate on one feature at a time without having to load context for
three different things in parallel.
Also, for each feature please provide a bit more background information
as well as reasons for why they are added.
> Signed-off-by: Nagarjuna Kristam
> ---
> drivers/phy/tegra/xusb-tegra210.c | 297
> +-
> drivers/phy/tegra/xusb.c | 75 +-
> drivers/phy/tegra/xusb.h | 16 +-
> include/linux/phy/tegra/xusb.h| 7 +-
> 4 files changed, 386 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/tegra/xusb-tegra210.c
> b/drivers/phy/tegra/xusb-tegra210.c
> index 05bee32..0289e10 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014, NVIDIA CORPORATION. All rights reserved.
> + * Copyright (c) 2014-2018, NVIDIA CORPORATION. All rights reserved.
> * Copyright (C) 2015 Google, Inc.
> *
> * This program is free software; you can redistribute it and/or modify it
> @@ -47,7 +47,10 @@
> #define XUSB_PADCTL_USB2_PAD_MUX_USB2_BIAS_PAD_XUSB 0x1
>
> #define XUSB_PADCTL_USB2_PORT_CAP 0x008
> +#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DISABLED(x) (0x0 << ((x) * 4))
> #define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_HOST(x) (0x1 << ((x) * 4))
> +#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_DEVICE(x) (0x2 << ((x) * 4))
> +#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_OTG(x) (0x3 << ((x) * 4))
> #define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_MASK(x) (0x3 << ((x) * 4))
>
> #define XUSB_PADCTL_SS_PORT_MAP 0x014
> @@ -55,6 +58,7 @@
> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_SHIFT(x) ((x) * 5)
> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(x) (0x7 << ((x) * 5))
> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(x, v) (((v) & 0x7) << ((x) * 5))
> +#define XUSB_PADCTL_SS_PORT_MAP_PORT_DISABLED 0x7
>
> #define XUSB_PADCTL_ELPG_PROGRAM1 0x024
> #define XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN (1 << 31)
> @@ -69,9 +73,14 @@
> #define XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(x) (1 << (1 + (x)))
> #define XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(x) (1 << (8 + (x)))
>
> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(x) (0x080 + (x) * 0x40)
> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIP (1 << 18)
> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIN (1 << 22)
> +
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(x) (0x084 + (x) * 0x40)
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_SHIFT 7
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
>
> #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)
> @@ -230,6 +239,12 @@
> #define XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(x) (0xa74 + (x) * 0x40)
> #define XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL 0xfcf01368
>
> +#define XUSB_PADCTL_USB2_VBUS_ID 0xc60
> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON (1 << 14)
> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT 18
> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_MASK 0xf
> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VAL 8
> +
> struct tegra210_xusb_fuse_calibration {
> u32 hs_curr_level[4];
> u32 hs_term_range_adj;
> @@ -905,6 +920,161 @@ static const struct tegra_xusb_lane_ops
> tegra210_usb2_lane_ops = {
> .remove = tegra210_usb2_lane_remove,
> };
>
> +/* must be called under padctl->lock */
> +static void tegra210_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl,
> + struct tegra_xusb_usb2_pad *pad)
> +{
> + u32 reg;
> +
> + if (pad->enable++ > 0)
> + return;
> +
> + dev_dbg(padctl->dev, "power on BIAS PAD & USB2 tracking\n");
> +
> + if (clk_prepare_enable(pad->clk))
> + dev_warn(padctl->dev, "failed to enable BIAS PAD & USB2
> tracking\n");
Maybe keep track of the exact error and make that part of the error
message?
> +
> + reg = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
> +
> + reg &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
> +(XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
> + reg |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
> + XUSB_PADCTL_USB2_BI
Re: [PATCH 6/8] dt-bindings: usb: Add NVIDIA Tegra XUSB device mode controller binding
On Thu, Jan 03, 2019 at 03:34:57PM +0530, Nagarjuna Kristam wrote:
> Add device-tree binding documentation for the XUSB device mode controller
> present on tegra210 SoC. This controller supports USB 3.0 specification
>
> Based on work by Andrew Bresticker .
>
> Signed-off-by: Nagarjuna Kristam
> ---
> .../devicetree/bindings/usb/nvidia,tegra-xudc.txt | 67
> ++
> 1 file changed, 67 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
> b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
> new file mode 100644
> index 000..244044b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
> @@ -0,0 +1,67 @@
> +Device tree binding for NVIDIA Tegra XUSB device mode controller (XUDC)
> +
Make the underline as wide as the title.
> +
> +The Tegra XUDC controller supports both USB 2.0 HighSpeed/FullSpeed and
> +USB 3.0 SuperSpeed protocols.
> +
> +Required properties:
> +
> +- compatible: For Tegra210, must contain "nvidia,tegra210-xudc".
> +- reg: Must contain the base and length of the XUSB device registers, XUSB
> device
> + PCI Config registers and XUSB device controller registers.
> +- interrupts: Must contain the XUSB device interrupt
> +- clocks: Must contain an entry for ell clocks used.
> + See ../clock/clock-bindings.txt for details.
> +- nvidia,xusb-padctl: phandle to the XUSB pad controller that is used to
> + configure the USB pads used by the XSUB controller
XSUB -> XUSB, though perhaps this should really be XUDC? Also, do we
really need the phandle to the XUSB pad controller? I think we have a
phandle to this for XUSB, but we need it for cases where we don't have
access to a PHY.
> +- power-domains: A list of PM domain specifiers that reference each
> power-domain
> + used by the XUSB device mode controller. This list must comprise of a
> specifier
> + for the XUSBA and XUSBB power-domains. See ../power/power_domain.txt and
> + ../arm/tegra/nvidia,tegra20-pmc.txt for details.
> +- power-domain-names: A list of names that represent each of the specifiers
> in
> + the 'power-domains' property. Must include 'xusb_ss' and 'xusb_device'
> +
> +For Tegra210:
> +- avddio_usb-supply: PCIe/USB3 analog logic power supply. Must supply 1.05 V.
avddio-usb-supply
> +- hvdd_usb-supply: USB controller power supply. Must supply 3.3 V.
hvdd-usb-supply
> +- avdd-pll-utmip-supply: UTMI PLL power supply. Must supply 1.8 V.
> +
> +- phys: Must contain an entry for each entry in phy-names.
> + See ../phy/phy-bindings.txt for details.
> +- extcon-cables: Must contains an extcon-cable entry which detects
> + USB VBUS pin. See ../extcon/extcon-usb-gpio.txt for details.
I think you use "extcon" in the DT change later in this series. You also
use "extcon" in the example later in this file.
> +
> +Optional properties:
> +
> +- phy-names: Should include an entry for each PHY used by the controller.
> + Names must be "usb2", and "usb3" if support SuperSpeed device mode.
> + - "usb3" phy, SuperSpeed (SSTX+/SSTX-/SSRX+/SSRX-) data lines
> + - "usb2" phy, USB 2.0 (D+/D-) data lines
> +
> +Example:
> +
> + extcon_usb: extcon_vbus {
> + compatible = "linux,extcon-usb-gpio";
> + vbus-gpio = <&gpio TEGRA_GPIO(Z, 0) 1>;
GPIO_ACTIVE_LOW for the third cell.
> + };
> + xudc@700d {
Space between the above. Also typically we'd have the extcon at the very
end of DT, so it may make sense to reflect that in the example, like so:
xudc@700d {
...
};
...
extcon_usb: extcon_vbus {
...
};
> + compatible = "nvidia,tegra210-xudc";
> + phys = <&{/padctl@7009f000/pads/usb2/lanes/usb2-0}>;
> + phy-names = "usb2;
> + power-domains = <&pd_xusbdev>, <&pd_xusbss>;
> + power-domain-names = "xusb_device", "xusb_ss";
> + avddio_usb-supply = <&vdd_pex_1v05>;
There's some indentation issue here. Also: avddio-usb-supply.
> + hvdd_usb-supply = <&vdd_3v3_sys>;
hvdd-usb-supply
> + avdd_pll_utmip-supply = <&vdd_1v8>;
avdd-pll-utmip-supply
> + reg = <0x0 0x700d 0x0 0x8000>,
> + <0x0 0x700d8000 0x0 0x1000>,
> + <0x0 0x700d9000 0x0 0x1000>;
> + interrupts = <0 44 0x4>;
> + clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>,
> + <&tegra_car TEGRA210_CLK_XUSB_SS>,
> + <&tegra_car TEGRA210_CLK_XUSB_SSP_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_HS_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_FS_SRC>;
It's typical for common properties to go first and have more exotic ones
later. See other device tree nodes and try to stay c
Re: [PATCH 5/8] arm64: tegra: Enable xudc on Jetson TX1
On Thu, Jan 03, 2019 at 03:34:56PM +0530, Nagarjuna Kristam wrote:
> Enable XUSB device mode driver for USB0 slot on Jetson TX1.
>
> Signed-off-by: Nagarjuna Kristam
> ---
> arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> index 985777c..293b95f 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> @@ -1325,6 +1325,21 @@
> status = "okay";
> };
>
> + extcon_usb: extcon_vbus {
> + compatible = "linux,extcon-usb-gpio";
> + vbus-gpio = <&gpio TEGRA_GPIO(Z, 0) 1>;
> + };
This needs to go to the end of the file. The ordering should be:
* all nodes with unit-address, sorted by unit-address
* all nodes without unit-address, sorted alphabetically
Thierry
signature.asc
Description: PGP signature
Re: [PATCH 4/8] arm64: tegra: Add xudc node for Tegra210
On Thu, Jan 03, 2019 at 03:34:55PM +0530, Nagarjuna Kristam wrote:
> Tegra210 has one XUSB device mode controller, which can be operated
> HS and SS modes. Add DT support for XUSB device mode controller.
>
> Signed-off-by: Nagarjuna Kristam
> ---
> arch/arm64/boot/dts/nvidia/tegra210.dtsi | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> index 8fe47d6..e34a865 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra210.dtsi
> @@ -885,6 +885,23 @@
> status = "disabled";
> };
>
> + xudc@700d {
> + compatible = "nvidia,tegra210-xudc";
> + reg = <0x0 0x700d 0x0 0x8000>,
> + <0x0 0x700d8000 0x0 0x1000>,
> + <0x0 0x700d9000 0x0 0x1000>;
> + interrupts = <0 44 0x4>;
GIC_SPI for the first cell, IRQ_TYPE_LEVEL_HIGH for the third cell.
> + power-domains = <&pd_xusbdev>, <&pd_xusbss>;
> + power-domain-names = "xusb_device", "xusb_ss";
> + clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>,
> + <&tegra_car TEGRA210_CLK_XUSB_SS>,
> + <&tegra_car TEGRA210_CLK_XUSB_SSP_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_HS_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_FS_SRC>;
I think it's slightly more idiomatic to have clocks before the power
domains. Also this should have a clock-names property that names the
clocks.
Other than that, this looks good to me.
Thierry
> + nvidia,xusb-padctl = <&padctl>;
> + status = "disabled";
> + };
> +
> padctl: padctl@7009f000 {
> compatible = "nvidia,tegra210-xusb-padctl";
> reg = <0x0 0x7009f000 0x0 0x1000>;
> --
> 2.7.4
>
signature.asc
Description: PGP signature
Re: [regression] USB power management failure to suspend / high CPU usage
On Mon, Feb 4, 2019 at 4:31 AM Mathias Nyman wrote: > > On 03.02.2019 04:08, Eric Blau wrote: > > > > I finally was able to complete the bisect on this issue. It seems that > > this commit introduced the regression on my MacBook Pro: > > > > cc8b329fef53c74a4abf98b0755b3832d572d6ce is the first bad commit > > commit cc8b329fef53c74a4abf98b0755b3832d572d6ce > > Author: Mathias Nyman > > Date: Thu Nov 15 11:38:41 2018 +0200 > > > > usb: xhci: Prevent bus suspend if a port connect change or polling > > state is detected > > > > commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. > > > > > > There's an additional fix for that patch, in 4.19 stable: > > commit e13bfb357f5bc04f9e7ccff7d07770388062a8cc > Author: Mathias Nyman > Date: Fri Dec 14 10:54:43 2018 +0200 > > xhci: Don't prevent USB2 bus suspend in state check intended for USB3 > only > > commit 45f750c16cae3625014c14c77bd9005eda975d35 upstream. > > If you already have that fix then one of the USB ports may be in a odd state > when suspending > > Does adding dynamic debug for xhci_hub_control show any messages when > suspending? > > mount -t debugfs none /sys/kernel/debug > echo -n 'func xhci_bus_suspend =p' > /sys/kernel/debug/dynamic_debug/control Hi Mathias, I see this behavior even in 4.20.6. With the above commit reverted, my laptop suspends and resumes properly again and I don't see the high CPU usage of the mentioned kernel threads. When I suspend with the dynamic debugging enabled on 4.20.6, I see a flood of xhci_hcd messages like the following: Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling There are so many of them that systemd-journald reports messages are lost too. From a bit earlier: Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 7 kernel messages Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 32 kernel messages Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 27 kernel messages Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 37 kernel messages then the suspend continues and fails: Feb 04 08:06:46 eric-macbookpro kernel: done. Feb 04 08:06:46 eric-macbookpro kernel: Freezing user space processes ... (elapsed 0.002 seconds) done. Feb 04 08:06:46 eric-macbookpro kernel: OOM killer disabled. Feb 04 08:06:46 eric-macbookpro kernel: Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. Feb 04 08:06:46 eric-macbookpro kernel: printk: Suspending console(s) (use no_console_suspend to debug) Feb 04 08:06:46 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:06:46 eric-macbookpro kernel: dpm_run_callback(): usb_dev_suspend+0x0/0x10 returns -16 Feb 04 08:06:46 eric-macbookpro kernel: PM: Device usb2 failed to suspend async: error -16 Feb 04 08:06:46 eric-macbookpro kernel: sd 0:0:0:0: [sda] Synchronizing SCSI cache Feb 04 08:06:46 eric-macbookpro kernel: sd 0:0:0:0: [sda] Stopping disk Feb 04 08:06:46 eric-macbookpro kernel: PM: Some devices failed to suspend, or early wake event detected Feb 04 08:06:46 eric-macbookpro kernel: pci :02:00.0: Refused to change power state, currently in D3
Re: [regression] USB power management failure to suspend / high CPU usage
On Mon, Feb 4, 2019 at 8:12 AM Eric Blau wrote: > > On Mon, Feb 4, 2019 at 4:31 AM Mathias Nyman > wrote: > > > > On 03.02.2019 04:08, Eric Blau wrote: > > > > > > I finally was able to complete the bisect on this issue. It seems that > > > this commit introduced the regression on my MacBook Pro: > > > > > > cc8b329fef53c74a4abf98b0755b3832d572d6ce is the first bad commit > > > commit cc8b329fef53c74a4abf98b0755b3832d572d6ce > > > Author: Mathias Nyman > > > Date: Thu Nov 15 11:38:41 2018 +0200 > > > > > > usb: xhci: Prevent bus suspend if a port connect change or polling > > > state is detected > > > > > > commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. > > > > > > > > > > There's an additional fix for that patch, in 4.19 stable: > > > > commit e13bfb357f5bc04f9e7ccff7d07770388062a8cc > > Author: Mathias Nyman > > Date: Fri Dec 14 10:54:43 2018 +0200 > > > > xhci: Don't prevent USB2 bus suspend in state check intended for USB3 > > only > > > > commit 45f750c16cae3625014c14c77bd9005eda975d35 upstream. > > > > If you already have that fix then one of the USB ports may be in a odd > > state when suspending > > > > Does adding dynamic debug for xhci_hub_control show any messages when > > suspending? > > > > mount -t debugfs none /sys/kernel/debug > > echo -n 'func xhci_bus_suspend =p' > /sys/kernel/debug/dynamic_debug/control > > Hi Mathias, > > I see this behavior even in 4.20.6. With the above commit reverted, my > laptop suspends and resumes properly again and I don't see the high > CPU usage of the mentioned kernel threads. > > When I suspend with the dynamic debugging enabled on 4.20.6, I see a > flood of xhci_hcd messages like the following: > > Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > > There are so many of them that systemd-journald reports messages are > lost too. From a bit earlier: > > Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 7 kernel > messages > Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 32 > kernel messages > Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 27 > kernel messages > Feb 04 08:06:31 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:31 eric-macbookpro systemd-journald[1132]: Missed 37 > kernel messages > > then the suspend continues and fails: > > Feb 04 08:06:46 eric-macbookpro kernel: done. > Feb 04 08:06:46 eric-macbookpro kernel: Freezing user space processes > ... (elapsed 0.002 seconds) done. > Feb 04 08:06:46 eric-macbookpro kernel: OOM killer disabled. > Feb 04 08:06:46 eric-macbookpro kernel: Freezing remaining freezable > tasks ... (elapsed 0.001 seconds) done. > Feb 04 08:06:46 eric-macbookpro kernel: printk: Suspending console(s) > (use no_console_suspend to debug) > Feb 04 08:06:46 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > suspend bailout, port in polling > Feb 04 08:06:46 eric-macbookpro kernel: dpm_run_callback(): > usb_dev_suspend+0x0/0x10 returns -16 > Feb 04 08:06:46 eric-macbookpro kernel: PM: Device usb2 failed to > suspend async: error -16 > Feb 04 08:06:46 eric-macbookpro kernel: sd 0:0:0:0: [sda] > Synchronizing SCSI cache > Feb 04 08:06:46 eric-macbookpro kernel: sd 0:0:0:0: [sda] Stopping disk > Feb 04 08:06:46 eric-macbookpro kernel: PM: Some devices failed to > suspend, or early wake event detected > Feb 04 08:06:46 eric-macbookpro kernel: pci :02:00.0: Refused to > change power state, currently in D3 BTW, when I see the CPU spinning at 100% for one kworker and ksoftirqd thread after a bad suspend attempt, those same messages are being emitted rapidly: PID USER PR NIVIRTRESSHR S %CPU %MEM TIME+ COMMAND 1132 root 20 0 698936 281144 279544 R 94.4 3.5 0:53.27 systemd-journal 42 root 20 0 0 0 0 R 74.1 0.0 0:37.59 kworker/2:1+pm 28 root 20 0 0 0 0 R 25.9 0.0 0:13.50 ksoftirqd/2 Feb 04 08:17:01 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:17:01 eric-macbookpro systemd-journald[22105]: Missed 2 kernel messages Feb 04 08:17:01 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Feb 04 08:17:01 eric-macbookpro systemd-journald[22105]: Missed 2 kernel messages Feb 04 08:17:01 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in
Re: [PATCH 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
On Thu, Jan 03, 2019 at 03:34:58PM +0530, Nagarjuna Kristam wrote: > This patch adds UDC driver for tegra XUSB 3.0 device mode controller. > XUSB device mode controller support SS, HS and FS modes > > Based on work by: > Mark Kuo > Andrew Bresticker > > Signed-off-by: Nagarjuna Kristam > --- > drivers/usb/gadget/udc/Kconfig | 11 + > drivers/usb/gadget/udc/Makefile |1 + > drivers/usb/gadget/udc/tegra_xudc.c | 3821 > +++ > 3 files changed, 3833 insertions(+) > create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c > > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig > index 0a16cbd..31665e7 100644 > --- a/drivers/usb/gadget/udc/Kconfig > +++ b/drivers/usb/gadget/udc/Kconfig > @@ -439,6 +439,17 @@ config USB_GADGET_XILINX > dynamically linked module called "udc-xilinx" and force all > gadget drivers to also be dynamically linked. > > +config USB_TEGRA_XUDC > + tristate "NVIDIA Superspeed USB 3.0 Device Controller" > + depends on ARCH_TEGRA > + default n "default n" is the default, so you can drop this line. > + help > + Enables TEGRA USB 3.0 device mode controller driver. > + > + Say "y" to link the driver statically, or "m" to build a > + dynamically linked module called "tegra_xudc" and force all "tegra-xudc", see below. > + gadget drivers to also be dynamically linked. > + > source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig" > > # > diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile > index 897f648..1c55c96 100644 > --- a/drivers/usb/gadget/udc/Makefile > +++ b/drivers/usb/gadget/udc/Makefile > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_BCM63XX_UDC) += bcm63xx_udc.o > obj-$(CONFIG_USB_FSL_USB2) += fsl_usb2_udc.o > fsl_usb2_udc-y := fsl_udc_core.o > fsl_usb2_udc-$(CONFIG_ARCH_MXC) += fsl_mxc_udc.o > +obj-$(CONFIG_USB_TEGRA_XUDC) += tegra_xudc.o We typically name drivers tegra-*, it'd be good to follow that here for consistency. > obj-$(CONFIG_USB_M66592) += m66592-udc.o > obj-$(CONFIG_USB_R8A66597) += r8a66597-udc.o > obj-$(CONFIG_USB_RENESAS_USB3) += renesas_usb3.o > diff --git a/drivers/usb/gadget/udc/tegra_xudc.c > b/drivers/usb/gadget/udc/tegra_xudc.c > new file mode 100644 > index 000..126c18e > --- /dev/null > +++ b/drivers/usb/gadget/udc/tegra_xudc.c > @@ -0,0 +1,3821 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * NVIDIA XUSB device mode controller > + * > + * Copyright (c) 2013-2018, NVIDIA CORPORATION. All rights reserved. > + * Copyright (c) 2015, Google Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include These don't seem to be sorted alphabetically. Also, pm_qos.h doesn't seem to be used here. > +#include I think this may only be used for the legacy tegra_powergate_*() functions, but those should be obsoleted by the use of generic power domains. > + > +/* XUSB_DEV registers */ > +#define SPARAM 0x000 > +#define SPARAM_ERSTMAX_SHIFT 16 > +#define SPARAM_ERSTMAX_MASK 0x1f > +#define DB 0x004 > +#define DB_TARGET_SHIFT 8 > +#define DB_TARGET_MASK 0xff > +#define DB_STREAMID_SHIFT 16 > +#define DB_STREAMID_MASK 0x > +#define ERSTSZ 0x008 > +#define ERSTSZ_ERSTXSZ_SHIFT(x) ((x) * 16) > +#define ERSTSZ_ERSTXSZ_MASK 0x > +#define ERSTXBALO(x) (0x010 + 8 * (x)) > +#define ERSTXBAHI(x) (0x014 + 8 * (x)) > +#define ERDPLO 0x020 > +#define ERDPLO_EHB BIT(3) > +#define ERDPHI 0x024 > +#define EREPLO 0x028 > +#define EREPLO_ECS BIT(0) > +#define EREPLO_SEGI BIT(1) > +#define EREPHI 0x02c > +#define CTRL 0x030 > +#define CTRL_RUN BIT(0) > +#define CTRL_LSE BIT(1) > +#define CTRL_IE BIT(4) > +#define CTRL_SMI_EVT BIT(5) > +#define CTRL_SMI_DSE BIT(6) > +#define CTRL_EWE BIT(7) > +#define CTRL_DEVADDR_SHIFT 24 > +#define CTRL_DEVADDR_MASK 0x7f > +#define CTRL_ENABLE BIT(31) > +#define ST 0x034 > +#define ST_RC BIT(0) > +#define ST_IP BIT(4) > +#define RT_IMOD 0x038 > +#define RT_IMOD_IMODI_SHIFT 0 > +#define RT_IMOD_IMODI_MASK 0x > +#define RT_IMOD_IMODC_SHIFT 16 > +#define RT_IMOD_IMODC_MASK 0x > +#define PORTSC 0x03c > +#define PORTSC_CCS BIT(0) > +#define PORTSC_PED BIT(1) > +#define PORTSC_PR BIT(4) > +#define PORTSC_PLS_SHIFT 5 > +#define PORTSC_PLS_MASK 0xf > +#define PORTSC_PLS_U0 0x0 > +#define PORTSC_PLS_U2 0x2 > +#define PORTSC_PLS_U3 0x3 > +#define PORTSC_PLS_DISABLED 0x4 > +#define PORTSC_PLS_RXDETECT 0x5 > +#define PORTSC_PLS_INACTIVE 0x6 > +#define PORTSC_PLS_RESUME 0xf > +#define PORTSC_PS_SHIFT 10 > +#define PORTSC_PS_MASK 0xf > +#define PORTSC_PS_UNDEFINED 0x0 > +#define PORTSC_PS_FS 0x1 > +#define PORTSC_
Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.
Hi,
Greg KH writes:
> On Thu, Jan 31, 2019 at 11:52:29AM +, Pawel Laszczak wrote:
>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>> to driver/usb/common/debug.c file. These moved functions include:
>> dwc3_decode_get_status
>> dwc3_decode_set_clear_feature
>> dwc3_decode_set_address
>> dwc3_decode_get_set_descriptor
>> dwc3_decode_get_configuration
>> dwc3_decode_set_configuration
>> dwc3_decode_get_intf
>> dwc3_decode_set_intf
>> dwc3_decode_synch_frame
>> dwc3_decode_set_sel
>> dwc3_decode_set_isoch_delay
>> dwc3_decode_ctrl
>>
>> These functions are used also in inroduced cdns3 driver.
>>
>> All functions prefixes were changed from dwc3 to usb.
>
> Ick, why?
moving dwc3-specific code into generic code.
>> Also, function's parameters has been extended according to the name
>> of fields in standard SETUP packet.
>> Additionally, patch adds usb_decode_ctrl function to
>> include/linux/usb/ch9.h file.
>
> Why ch9.h? It's not something that is specified in the spec, it's a
> usb-specific thing :)
agree.
>> +/**
>> + * usb_decode_ctrl - Returns human readable representation of control
>> request.
>> + * @str: buffer to return a human-readable representation of control
>> request.
>> + * This buffer should have about 200 bytes.
>
> "about 200 bytes" is not very specific.
>
> Pass in the length so we know we don't overflow it.
agree.
>> + * @bRequestType: matches the USB bmRequestType field
>> + * @bRequest: matches the USB bRequest field
>> + * @wValue: matches the USB wValue field (CPU byte order)
>> + * @wIndex: matches the USB wIndex field (CPU byte order)
>> + * @wLength: matches the USB wLength field (CPU byte order)
>> + *
>> + * Function returns decoded, formatted and human-readable description of
>> + * control request packet.
>> + *
>> + * Important: wValue, wIndex, wLength parameters before invoking this
>> function
>> + * should be processed by le16_to_cpu macro.
>> + */
>> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
>> +__u16 wValue, __u16 wIndex, __u16 wLength);
>
> Why are you returning a value, isn't the data stored in str? Why not
> just return an int saying if the call succeeded or not?
There is one detail here. The usage scenario for this is for
tracepoints. When dealing with tracepoints we want to delay decoding of
the data into strings until print-time. I guess it's best to illustrate
with an example:
DECLARE_EVENT_CLASS(dwc3_log_ctrl,
TP_PROTO(struct usb_ctrlrequest *ctrl),
TP_ARGS(ctrl),
TP_STRUCT__entry(
__field(__u8, bRequestType)
__field(__u8, bRequest)
__field(__u16, wValue)
__field(__u16, wIndex)
__field(__u16, wLength)
__dynamic_array(char, str, DWC3_MSG_MAX)
),
TP_fast_assign(
__entry->bRequestType = ctrl->bRequestType;
__entry->bRequest = ctrl->bRequest;
__entry->wValue = le16_to_cpu(ctrl->wValue);
__entry->wIndex = le16_to_cpu(ctrl->wIndex);
__entry->wLength = le16_to_cpu(ctrl->wLength);
),
TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
__entry->bRequestType,
__entry->bRequest, __entry->wValue,
__entry->wIndex, __entry->wLength)
)
);
The above is the code is today (well, I've added buffer size as an
argument). If I make dwc3_decode_ctrl() return an integer, I can't call
it from TP_printk() time. I'd have to move it to TP_fast_assign() time
which is supposed to be, simply, a copy of the necessary data. IOW, I
would have this:
DECLARE_EVENT_CLASS(dwc3_log_ctrl,
TP_PROTO(struct usb_ctrlrequest *ctrl),
TP_ARGS(ctrl),
TP_STRUCT__entry(
__dynamic_array(char, str, DWC3_MSG_MAX)
),
TP_fast_assign(
dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
ctrl->bRequestType,
ctrl->bRequest,
le16_to_cpu(ctrl->wValue),
le16_to_cpu(ctrl->wIndex),
le16_to_cpu(ctrl->wLength));
),
TP_printk("%s", __get_str(str)
)
);
Essentially, we end up moving decoding of the tracepoint to the time it
is captured; IOW, we reintroduce regular latency of string formatting.
What we could do, is move all functions called by dwc3_decode_ctrl() to
return int, but leave dwc3_decode_ctrl() returning a pointer to str just
so we continue decoding the data at printing time.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver.
On Mon, Feb 04, 2019 at 04:17:00PM +0200, Felipe Balbi wrote:
>
> Hi,
>
> Greg KH writes:
> > On Thu, Jan 31, 2019 at 11:52:29AM +, Pawel Laszczak wrote:
> >> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
> >> to driver/usb/common/debug.c file. These moved functions include:
> >> dwc3_decode_get_status
> >> dwc3_decode_set_clear_feature
> >> dwc3_decode_set_address
> >> dwc3_decode_get_set_descriptor
> >> dwc3_decode_get_configuration
> >> dwc3_decode_set_configuration
> >> dwc3_decode_get_intf
> >> dwc3_decode_set_intf
> >> dwc3_decode_synch_frame
> >> dwc3_decode_set_sel
> >> dwc3_decode_set_isoch_delay
> >> dwc3_decode_ctrl
> >>
> >> These functions are used also in inroduced cdns3 driver.
> >>
> >> All functions prefixes were changed from dwc3 to usb.
> >
> > Ick, why?
>
> moving dwc3-specific code into generic code.
That says what it does, but not why :)
And if this really was just moving things around, why was only one
symbol needed to be exported and not all of them?
> >> + * @bRequestType: matches the USB bmRequestType field
> >> + * @bRequest: matches the USB bRequest field
> >> + * @wValue: matches the USB wValue field (CPU byte order)
> >> + * @wIndex: matches the USB wIndex field (CPU byte order)
> >> + * @wLength: matches the USB wLength field (CPU byte order)
> >> + *
> >> + * Function returns decoded, formatted and human-readable description of
> >> + * control request packet.
> >> + *
> >> + * Important: wValue, wIndex, wLength parameters before invoking this
> >> function
> >> + * should be processed by le16_to_cpu macro.
> >> + */
> >> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
> >> + __u16 wValue, __u16 wIndex, __u16 wLength);
> >
> > Why are you returning a value, isn't the data stored in str? Why not
> > just return an int saying if the call succeeded or not?
>
> There is one detail here. The usage scenario for this is for
> tracepoints. When dealing with tracepoints we want to delay decoding of
> the data into strings until print-time. I guess it's best to illustrate
> with an example:
>
> DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __field(__u8, bRequestType)
> __field(__u8, bRequest)
> __field(__u16, wValue)
> __field(__u16, wIndex)
> __field(__u16, wLength)
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> __entry->bRequestType = ctrl->bRequestType;
> __entry->bRequest = ctrl->bRequest;
> __entry->wValue = le16_to_cpu(ctrl->wValue);
> __entry->wIndex = le16_to_cpu(ctrl->wIndex);
> __entry->wLength = le16_to_cpu(ctrl->wLength);
> ),
> TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> __entry->bRequestType,
> __entry->bRequest, __entry->wValue,
> __entry->wIndex, __entry->wLength)
> )
> );
>
> The above is the code is today (well, I've added buffer size as an
> argument). If I make dwc3_decode_ctrl() return an integer, I can't call
> it from TP_printk() time. I'd have to move it to TP_fast_assign() time
> which is supposed to be, simply, a copy of the necessary data. IOW, I
> would have this:
>
> DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> ctrl->bRequestType,
> ctrl->bRequest,
> le16_to_cpu(ctrl->wValue),
> le16_to_cpu(ctrl->wIndex),
> le16_to_cpu(ctrl->wLength));
> ),
> TP_printk("%s", __get_str(str)
> )
> );
>
> Essentially, we end up moving decoding of the tracepoint to the time it
> is captured; IOW, we reintroduce regular latency of string formatting.
>
> What we could do, is move all functions called by dwc3_decode_ctrl() to
> return int, but leave dwc3_decode_ctrl() returning a pointer to str just
> so we continue decoding the data at printing time.
Ok, it wasn't obvious that this was used in a tracepoint like this, that
makes more sense.
So, it should be documented as well :)
thanks,
greg k-h
Re: [regression] USB power management failure to suspend / high CPU usage
On 04.02.2019 15:17, Eric Blau wrote: On Mon, Feb 4, 2019 at 8:12 AM Eric Blau wrote: On Mon, Feb 4, 2019 at 4:31 AM Mathias Nyman wrote: On 03.02.2019 04:08, Eric Blau wrote: I finally was able to complete the bisect on this issue. It seems that this commit introduced the regression on my MacBook Pro: cc8b329fef53c74a4abf98b0755b3832d572d6ce is the first bad commit commit cc8b329fef53c74a4abf98b0755b3832d572d6ce Author: Mathias Nyman Date: Thu Nov 15 11:38:41 2018 +0200 usb: xhci: Prevent bus suspend if a port connect change or polling state is detected commit 2f31a67f01a8beb22cae754c53522cb61a005750 upstream. There's an additional fix for that patch, in 4.19 stable: commit e13bfb357f5bc04f9e7ccff7d07770388062a8cc Author: Mathias Nyman Date: Fri Dec 14 10:54:43 2018 +0200 xhci: Don't prevent USB2 bus suspend in state check intended for USB3 only commit 45f750c16cae3625014c14c77bd9005eda975d35 upstream. If you already have that fix then one of the USB ports may be in a odd state when suspending Does adding dynamic debug for xhci_hub_control show any messages when suspending? mount -t debugfs none /sys/kernel/debug echo -n 'func xhci_bus_suspend =p' > /sys/kernel/debug/dynamic_debug/control Hi Mathias, I see this behavior even in 4.20.6. With the above commit reverted, my laptop suspends and resumes properly again and I don't see the high CPU usage of the mentioned kernel threads. When I suspend with the dynamic debugging enabled on 4.20.6, I see a flood of xhci_hcd messages like the following: Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Ok, 4.20.6 has the 45f750c16cae "Don't prevent USB2 bus suspend" fix in, so seems a USB3 port is stuck in polling mode in some cases, preventing suspend. Original patch was meant to prevent runtime suspend in case a newly attached USB3 device was slow to enumerate, but same code is called for system suspend. The original patch could be limited to runtime suspend only, but looks like that requires usb core change. hcd->driver->bus_suspend(hcd) callback needs to provide pm_message_t as a parameter. Alan, would it make sense to let host controller drivers decide to prevent bus_suspend based on port states and pm message type (runtime or system suspend)? -Mathias
Re: [regression] USB power management failure to suspend / high CPU usage
On Mon, 4 Feb 2019, Mathias Nyman wrote: > >> Hi Mathias, > >> > >> I see this behavior even in 4.20.6. With the above commit reverted, my > >> laptop suspends and resumes properly again and I don't see the high > >> CPU usage of the mentioned kernel threads. > >> > >> When I suspend with the dynamic debugging enabled on 4.20.6, I see a > >> flood of xhci_hcd messages like the following: > >> > >> Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus > >> suspend bailout, port in polling > > Ok, 4.20.6 has the 45f750c16cae "Don't prevent USB2 bus suspend" fix in, > so seems a USB3 port is stuck in polling mode in some cases, preventing > suspend. > > Original patch was meant to prevent runtime suspend in case a newly attached > USB3 > device was slow to enumerate, but same code is called for system suspend. > > The original patch could be limited to runtime suspend only, but looks like > that > requires usb core change. > hcd->driver->bus_suspend(hcd) callback needs to provide pm_message_t as a > parameter. > > Alan, would it make sense to let host controller drivers decide to prevent > bus_suspend > based on port states and pm message type (runtime or system suspend)? I would prefer to see the underlying cause for this problem figured out. Why is the USB3 port stuck in polling mode? Is it a hardware issue or a software bug? More detailed debugging logs might help. Alan Stern
Re: [regression] USB power management failure to suspend / high CPU usage
On 04.02.2019 17:27, Alan Stern wrote: On Mon, 4 Feb 2019, Mathias Nyman wrote: Hi Mathias, I see this behavior even in 4.20.6. With the above commit reverted, my laptop suspends and resumes properly again and I don't see the high CPU usage of the mentioned kernel threads. When I suspend with the dynamic debugging enabled on 4.20.6, I see a flood of xhci_hcd messages like the following: Feb 04 08:06:44 eric-macbookpro kernel: xhci_hcd :00:14.0: Bus suspend bailout, port in polling Ok, 4.20.6 has the 45f750c16cae "Don't prevent USB2 bus suspend" fix in, so seems a USB3 port is stuck in polling mode in some cases, preventing suspend. Original patch was meant to prevent runtime suspend in case a newly attached USB3 device was slow to enumerate, but same code is called for system suspend. The original patch could be limited to runtime suspend only, but looks like that requires usb core change. hcd->driver->bus_suspend(hcd) callback needs to provide pm_message_t as a parameter. Alan, would it make sense to let host controller drivers decide to prevent bus_suspend based on port states and pm message type (runtime or system suspend)? I would prefer to see the underlying cause for this problem figured out. Why is the USB3 port stuck in polling mode? Is it a hardware issue or a software bug? More detailed debugging logs might help. Eric, can you take logs of this? For adding xhci tracing mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable after failed suspend attempt show /sys/kernel/debug/tracing/trace For adding xhci and usb core dynamic debug: echo -n 'module xhci_hcd =p' > /sys/kernel/debug/dynamic_debug/control echo -n 'module usbcore =p' >/sys/kernel/debug/dynamic_debug/control -Mathias
Re: [PATCH] USB: serial: cp210x: add minimun baud rate for CP2105 SCI
On Mon, Feb 04, 2019 at 12:21:30PM +0100, Johanna Abrahamsson wrote:
> From: Johanna Abrahamsson
>
> This patch adds a minimum baud rate of 2400 for CP2105 SCI
>
> According to the datasheet for CP2105, the SCI supports 2400 as the
> lowest baud rate. As this is not heeded in the current code, an error
> message 'failed set req 0x1e size 4 status: -32' when trying to set a
> lower baud rate such as 300.
Good to hear to you found the root cause of that failed baudrate
request.
> Since this is, as far as I can tell, the only cp210x chip with a minimum
> baud rate higher than 300, I've added a special case to
> cp210x_change_speed rather than adding a min_speed field in
> cp210x_serial_private.
But please do add a min_speed field instead of special casing which
tends to make the code harder to read.
> Signed-off-by: Johanna Abrahamsson
> ---
> drivers/usb/serial/cp210x.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index 336a3c0f9f2c..181abf7bb8c0 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -,9 +,12 @@ static void cp210x_change_speed(struct tty_struct *tty,
>*/
> if (priv->use_actual_rate)
> baud = cp210x_get_actual_rate(serial, baud);
> - else if (baud < 100)
> + else if (baud < 100) {
> + if (priv->partnum == CP210X_PARTNUM_CP2105 &&
> + cp210x_interface_num(serial) == 1)
> + baud = clamp(baud, 2400u, priv->max_speed);
> baud = cp210x_get_an205_rate(baud);
We already have a clamp in place in cp210x_get_actual_rate() so just add
a similar construct to cp210x_get_actual_rate() and amend
cp210x_init_max_speed() as needed.
> - else if (baud > priv->max_speed)
> + } else if (baud > priv->max_speed)
> baud = priv->max_speed;
>
> dev_dbg(&port->dev, "%s - setting baud rate to %u\n", __func__, baud);
Johan
Re: [PATCH] USB: serial: cp210x: add minimun baud rate for CP2105 SCI
On Mon, Feb 04, 2019 at 04:57:00PM +0100, Johan Hovold wrote:
> On Mon, Feb 04, 2019 at 12:21:30PM +0100, Johanna Abrahamsson wrote:
> > From: Johanna Abrahamsson
> >
> > This patch adds a minimum baud rate of 2400 for CP2105 SCI
> >
> > According to the datasheet for CP2105, the SCI supports 2400 as the
> > lowest baud rate. As this is not heeded in the current code, an error
> > message 'failed set req 0x1e size 4 status: -32' when trying to set a
> > lower baud rate such as 300.
>
> Good to hear to you found the root cause of that failed baudrate
> request.
>
> > Since this is, as far as I can tell, the only cp210x chip with a minimum
> > baud rate higher than 300, I've added a special case to
> > cp210x_change_speed rather than adding a min_speed field in
> > cp210x_serial_private.
>
> But please do add a min_speed field instead of special casing which
> tends to make the code harder to read.
>
> > Signed-off-by: Johanna Abrahamsson
> > ---
> > drivers/usb/serial/cp210x.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index 336a3c0f9f2c..181abf7bb8c0 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -,9 +,12 @@ static void cp210x_change_speed(struct tty_struct
> > *tty,
> > */
> > if (priv->use_actual_rate)
> > baud = cp210x_get_actual_rate(serial, baud);
> > - else if (baud < 100)
> > + else if (baud < 100) {
> > + if (priv->partnum == CP210X_PARTNUM_CP2105 &&
> > + cp210x_interface_num(serial) == 1)
> > + baud = clamp(baud, 2400u, priv->max_speed);
> > baud = cp210x_get_an205_rate(baud);
>
> We already have a clamp in place in cp210x_get_actual_rate() so just add
> a similar construct to cp210x_get_actual_rate() and amend
> cp210x_init_max_speed() as needed.
Or better still, move the clamp from cp210x_get_actual_rate() to above
these conditionals and make sure to honour min_speed. Then you can drop
the final else-if here too.
> > - else if (baud > priv->max_speed)
> > + } else if (baud > priv->max_speed)
> > baud = priv->max_speed;
> >
> > dev_dbg(&port->dev, "%s - setting baud rate to %u\n", __func__, baud);
Johan
Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend
On Tue, Jan 15, 2019 at 11:29:42AM +0100, Johan Hovold wrote:
> On Tue, Jan 15, 2019 at 10:26:07AM +0100, Johan Hovold wrote:
> > On Tue, Jan 15, 2019 at 09:17:58AM +, Karoly Pados wrote:
> > > > I think it's better to add the autopm call to gpio210x_gpio_get/set
> > > > only. This will allow for a simpler patch, and keeps the autopm handling
> > > > confined to the gpio paths.
> > >
> > > I'll submit a v2.
> > >
> > > >> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip
> > > >> *gc, unsigned int gpio, int
> > > >> value)
> > > >> } else {
> > > >> u16 wIndex = buf.state << 8 | buf.mask;
> > > >>
> > > >> + usb_autopm_get_interface(serial->interface);
> > > >
> > > > Also make sure to always check for errors from autopm_get().
> > >
> > > I checked everywhere else, the reason I didn't check here is on
> > > purpose based on your previous feedback. The caller function here
> > > doesn't have a return value, so the only way to return errors is to
> > > log, but in my last patch to ftdi_sio you made clear that errors from
> > > autopm_get shouldn't get logged. Trying to call usb_control_msg() even
> > > though the device could not wake does not cause issues, and the return
> > > value from usb_control_msg() clearly identifies the reason for failure
> > > (failure due to autosuspend), so error information is not lost either.
> > > So I thought not checking here has no real disadvantage and I still
> > > stay conformant to your previous guidance.
> >
> > Ok, I understand your reasoning, but please do check for errors and bail
> > out early if autopm_get() fails. No need to log errors.
>
> Actually, we should probably add the missing error handling to the
> callers and have gpio_set() propagate errors too. If you want to take a
> stab at that, that could be a follow-on patch.
Karoly, did you plan on sending a v2 of this one?
Thanks,
Johan
Re: [PATCH] USB: serial: cp210x: add GPIO support for CP2104
On Mon, Jan 28, 2019 at 01:57:17PM +0800, Icenowy Zheng wrote: > The CP2104 chips feature 4 controllable GPIO pins, which are similar to > the ones on CP2102N chip (output-only when push-pull, output or > simulated input mode when open-drain). > > Add support for the GPIO pins for cp210x driver. The pin get/set routine > is shared with CP2102N, but the pinconf initialization code is not > shared because the acquisition of GPIO configuration in OTP ROM is > similar to CP2105, not CP2102N. > > Signed-off-by: Icenowy Zheng Thanks for the patch, all nice and clean. Now applied. Johan
[PATCH 2/3] udc: net2280: Fix overrun of OUT messages
From: Guido Kiener The OUT endpoint normally blocks (NAK) subsequent packets when a short packet is received and returns an incomplete queue entry to the gadget driver. Thereby the gadget driver can detect a short packet when reading queue entries with a length that is not equal to a multiple of packet size. The start_queue() function enables receiving OUT packets regardless of the content of the OUT FIFO. This results in a problem: When receiving is enabled more OUT packets are appended to the last short packet and the gadget driver cannot determine the end of a short packet anymore. Furthermore the remaining data in the OUT FIFO is not delivered to the gadget driver immediately and can produce timeout errors. This fix only stops OUT naking when all FIFO data is delivered to the gadget driver and the OUT FIFO is empty. Signed-off-by: Guido Kiener --- drivers/usb/gadget/udc/net2280.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 7154f00dea40..1cb58fd5d1c6 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -867,7 +867,7 @@ static void start_queue(struct net2280_ep *ep, u32 dmactl, u32 td_dma) writel(BIT(DMA_START), &dma->dmastat); - if (!ep->is_in) + if (!ep->is_in && readl(&ep->regs->ep_avail) == 0) stop_out_naking(ep); } -- 2.17.1
[PATCH 3/3] udc: net2280: Fix typo
From: Guido Kiener
Fix spelling of automatically.
Signed-off-by: Guido Kiener
---
drivers/usb/gadget/udc/net2280.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 1cb58fd5d1c6..430d582d8b6e 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -912,7 +912,7 @@ static void start_dma(struct net2280_ep *ep, struct
net2280_request *req)
tmp = dmactl_default;
/* force packet boundaries between dma requests, but prevent the
-* controller from automagically writing a last "short" packet
+* controller from automatically writing a last "short" packet
* (zero length) unless the driver explicitly said to do that.
*/
if (ep->is_in) {
--
2.17.1
[PATCH 1/3] udc: net2280: Fix net2280_disable
From: Guido Kiener
A reset e.g. calling ep_reset_338x() can happen while endpoints
are enabled. The ep_reset_338x() sets ep->desc = NULL to mark
endpoint being invalid. A subsequent call of net2280_disable will
fail and return -EINVAL to parent function usb_ep_disable(),
which will fail, too, and do not set the member ep->enabled = false.
See:
https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/core.c#L139
This fix ignores dp->desc and allows net2280_disable() to succeed.
Subsequent calls to usb_ep_enable()/usb_ep_disable() succeeds.
Signed-off-by: Guido Kiener
---
drivers/usb/gadget/udc/net2280.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index e7dae5379e04..7154f00dea40 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -516,8 +516,8 @@ static int net2280_disable(struct usb_ep *_ep)
unsigned long flags;
ep = container_of(_ep, struct net2280_ep, ep);
- if (!_ep || !ep->desc || _ep->name == ep0name) {
- pr_err("%s: Invalid ep=%p or ep->desc\n", __func__, _ep);
+ if (!_ep || _ep->name == ep0name) {
+ pr_err("%s: Invalid ep=%p\n", __func__, _ep);
return -EINVAL;
}
spin_lock_irqsave(&ep->dev->lock, flags);
--
2.17.1
Re: [PATCH 1/3] udc: net2280: Fix net2280_disable
On Mon, 4 Feb 2019, Guido Kiener wrote:
> From: Guido Kiener
>
> A reset e.g. calling ep_reset_338x() can happen while endpoints
> are enabled.
How can this happen? That routine is called from only two places.
One is in net2280_disable(), so after the endpoint has already been
disabled. The other is in usb_reinit_338x() via usb_reinit() via
stop_activity(), which disconnects the gadget driver and thereby
disables all the endpoints.
> The ep_reset_338x() sets ep->desc = NULL to mark
> endpoint being invalid. A subsequent call of net2280_disable will
> fail and return -EINVAL to parent function usb_ep_disable(),
> which will fail, too, and do not set the member ep->enabled = false.
So maybe a better fix (assuming there really is a problem) is to make
usb_ep_disable() clear ep->enabled always. After all, the only
reasonable way for usb_ep_disable() to fail is if the endpoint isn't
enabled in the first place.
Alan Stern
> See:
> https://elixir.bootlin.com/linux/v5.0-rc5/source/drivers/usb/gadget/udc/core.c#L139
>
> This fix ignores dp->desc and allows net2280_disable() to succeed.
> Subsequent calls to usb_ep_enable()/usb_ep_disable() succeeds.
>
> Signed-off-by: Guido Kiener
> ---
> drivers/usb/gadget/udc/net2280.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/net2280.c
> b/drivers/usb/gadget/udc/net2280.c
> index e7dae5379e04..7154f00dea40 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -516,8 +516,8 @@ static int net2280_disable(struct usb_ep *_ep)
> unsigned long flags;
>
> ep = container_of(_ep, struct net2280_ep, ep);
> - if (!_ep || !ep->desc || _ep->name == ep0name) {
> - pr_err("%s: Invalid ep=%p or ep->desc\n", __func__, _ep);
> + if (!_ep || _ep->name == ep0name) {
> + pr_err("%s: Invalid ep=%p\n", __func__, _ep);
> return -EINVAL;
> }
> spin_lock_irqsave(&ep->dev->lock, flags);
>
Re: [PATCH 2/3] udc: net2280: Fix overrun of OUT messages
On Mon, 4 Feb 2019, Guido Kiener wrote: > From: Guido Kiener > > The OUT endpoint normally blocks (NAK) subsequent packets when a > short packet is received and returns an incomplete queue entry to > the gadget driver. Thereby the gadget driver can detect > a short packet when reading queue entries with a length that is > not equal to a multiple of packet size. > > The start_queue() function enables receiving OUT packets regardless > of the content of the OUT FIFO. This results in a problem: > When receiving is enabled more OUT packets are appended to the last > short packet and the gadget driver cannot determine the end of a > short packet anymore. Furthermore the remaining data in the OUT FIFO > is not delivered to the gadget driver immediately and can produce > timeout errors. How can that happen? When the short packet is received, the endpoint immediately starts sending NAKs, so no more data can enter the FIFO. Furthermore, the incomplete transfer is returned to the gadget driver, which removes the short packet from the FIFO. So the FIFO has to be empty when start_queue() is called. Alan Stern > This fix only stops OUT naking when all FIFO data is delivered > to the gadget driver and the OUT FIFO is empty. > > Signed-off-by: Guido Kiener > --- > drivers/usb/gadget/udc/net2280.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/net2280.c > b/drivers/usb/gadget/udc/net2280.c > index 7154f00dea40..1cb58fd5d1c6 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -867,7 +867,7 @@ static void start_queue(struct net2280_ep *ep, u32 > dmactl, u32 td_dma) > > writel(BIT(DMA_START), &dma->dmastat); > > - if (!ep->is_in) > + if (!ep->is_in && readl(&ep->regs->ep_avail) == 0) > stop_out_naking(ep); > }
Re: [PATCH 3/3] udc: net2280: Fix typo
On Mon, 4 Feb 2019, Guido Kiener wrote:
> From: Guido Kiener
>
> Fix spelling of automatically.
>
> Signed-off-by: Guido Kiener
> ---
> drivers/usb/gadget/udc/net2280.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/net2280.c
> b/drivers/usb/gadget/udc/net2280.c
> index 1cb58fd5d1c6..430d582d8b6e 100644
> --- a/drivers/usb/gadget/udc/net2280.c
> +++ b/drivers/usb/gadget/udc/net2280.c
> @@ -912,7 +912,7 @@ static void start_dma(struct net2280_ep *ep, struct
> net2280_request *req)
> tmp = dmactl_default;
>
> /* force packet boundaries between dma requests, but prevent the
> - * controller from automagically writing a last "short" packet
> + * controller from automatically writing a last "short" packet
This is not a misspelling. It is deliberate:
http://www.hacker-dictionary.com/terms/automagically
Alan Stern
>* (zero length) unless the driver explicitly said to do that.
>*/
> if (ep->is_in) {
>
Re: [PATCH v2 2/2] PCI: Override Synopsys USB 3.x HAPS device class
On Mon, 2018-12-10 at 14:08 -0800, Thinh Nguyen wrote:
> Synopsys USB 3.x host HAPS platform has a class code of
> PCI_CLASS_SERIAL_USB_XHCI, and xhci driver can claim it. However, these
> devices should use dwc3-haps driver. Change these devices' class code to
> PCI_CLASS_SERIAL_USB_DEVICE to prevent the xhci-pci driver from claiming
> them.
>
> +
> + switch (pdev->device) {
> + case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
> + case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
> + case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31:
> + pdev->class = PCI_CLASS_SERIAL_USB_DEVICE;
> + pci_info(pdev, "PCI class overridden (%#08x -> %#08x) so dwc3
> driver can claim this instead of xhci\n",
> + class, pdev->class);
> + break;
> + default:
> + return;
> + }
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
> + quirk_synopsys_haps);
> +
This change breaks my IMX7d based device. This SoC has a PCIe bus
based on the Synopsys Designware host controller. This has a root
bridge that shows up as:
00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00 [Normal
decode])
00:00.0 0604: 16c3:abcd (rev 01) (prog-if 00 [Normal decode])
Which is to say, class 0x0604, vendor PCI_VENDOR_ID_SYNOPSYS, and the
device ID 0xabcd.
It looks like there has been a bit of sloppy allocation of PCI device
codes, as PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is also 0xabcd.
So the result of this patch is to try to turn the imx7d PCIe root
bridge into a USB controller. Which of course breaks it and prevents
anything behind it, i.e. the endpoint attached to the pci-e bus, from
working.
Somehow this quirk needs to be more targeted.
Re: [PATCH v2 2/2] PCI: Override Synopsys USB 3.x HAPS device class
Hi Trent,
Trent Piepho wrote:
> On Mon, 2018-12-10 at 14:08 -0800, Thinh Nguyen wrote:
>> Synopsys USB 3.x host HAPS platform has a class code of
>> PCI_CLASS_SERIAL_USB_XHCI, and xhci driver can claim it. However, these
>> devices should use dwc3-haps driver. Change these devices' class code to
>> PCI_CLASS_SERIAL_USB_DEVICE to prevent the xhci-pci driver from claiming
>> them.
>>
>> +
>> +switch (pdev->device) {
>> +case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
>> +case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
>> +case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB31:
>> +pdev->class = PCI_CLASS_SERIAL_USB_DEVICE;
>> +pci_info(pdev, "PCI class overridden (%#08x -> %#08x) so dwc3
>> driver can claim this instead of xhci\n",
>> + class, pdev->class);
>> +break;
>> +default:
>> +return;
>> +}
>> +}
>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>> + quirk_synopsys_haps);
>> +
> This change breaks my IMX7d based device. This SoC has a PCIe bus
> based on the Synopsys Designware host controller. This has a root
> bridge that shows up as:
>
> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00 [Normal
> decode])
> 00:00.0 0604: 16c3:abcd (rev 01) (prog-if 00 [Normal decode])
>
>
> Which is to say, class 0x0604, vendor PCI_VENDOR_ID_SYNOPSYS, and the
> device ID 0xabcd.
>
> It looks like there has been a bit of sloppy allocation of PCI device
> codes, as PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is also 0xabcd.
>
> So the result of this patch is to try to turn the imx7d PCIe root
> bridge into a USB controller. Which of course breaks it and prevents
> anything behind it, i.e. the endpoint attached to the pci-e bus, from
> working.
>
> Somehow this quirk needs to be more targeted.
Right. Lukas also reported this. It appears that there's a duplicate VID
PID used for the USB controller on HAPS platform. You can review the
discussion subject "Linux Kernel Regression: HAPS quirk breaks PCIe on
i.MX6QP".
I'll send out a patch to resolve this issue. You can check the change to
see if this resolves your issue:
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b0a413f3f7ca..f46e7de9e15d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -639,8 +639,8 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
break;
}
}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
-quirk_synopsys_haps);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
+ PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps);
/*
* Let's make the southbridge information explicit instead of having to
Thanks,
Thinh
Re: [PATCH v2 2/2] PCI: Override Synopsys USB 3.x HAPS device class
On Mon, 2019-02-04 at 23:18 +, Thinh Nguyen wrote: > Trent Piepho wrote: > > On Mon, 2018-12-10 at 14:08 -0800, Thinh Nguyen wrote: > > > Synopsys USB 3.x host HAPS platform has a class code of > > > PCI_CLASS_SERIAL_USB_XHCI, and xhci driver can claim it. However, these > > > devices should use dwc3-haps driver. Change these devices' class code to > > > PCI_CLASS_SERIAL_USB_DEVICE to prevent the xhci-pci driver from claiming > > > them. > > > > > This change breaks my IMX7d based device. This SoC has a PCIe bus > > based on the Synopsys Designware host controller. This has a root > > > > > Right. Lukas also reported this. It appears that there's a duplicate VID > PID used for the USB controller on HAPS platform. You can review the > discussion subject "Linux Kernel Regression: HAPS quirk breaks PCIe on > i.MX6QP". > > -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID, > -quirk_synopsys_haps); > +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID, > + PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps); Thanks for the quick response. Tested this on imx7d, and as expected, it fixes the problem. The host bridge has class PCI_CLASS_BRIDGE_PCI like it should and the quirk no longer matches.
Re: Frequent dwc3 crashes on suspend or reboot since 5.0-rc1
On Sat, Feb 2, 2019 at 9:00 AM Alan Stern wrote:
>
> On Fri, 1 Feb 2019, John Stultz wrote:
>
> > Hey all,
> > Since the 5.0 merge window opened, I've been tripping on frequent
> > dwc3 crashes on reboot and suspend, which I've added an example to the
> > bottom of this mail.
> >
> > I've dug in a little bit and sort of have a sense of whats going on.
> >
> > In ffs_epfile_io():
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/function/f_fs.c#n1065
> >
> > The completion done is setup on the stack:
> > DECLARE_COMPLETION_ONSTACK(done);
> >
> > Then later we setup a request and queue it:
> > req->context = &done;
> > ...
> > ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
> >
> > Then wait for it:
> > if (unlikely(wait_for_completion_interruptible(&done))) {
> > /*
> > * To avoid race condition with ffs_epfile_io_complete,
> > * dequeue the request first then check
> > * status. usb_ep_dequeue API should guarantee no race
> > * condition with req->complete callback.
> > */
> > usb_ep_dequeue(ep->ep, req);
>
> This code contains a bug: It assumes that usb_ep_dequeue() waits until
> the request has been completed. You should insert
>
> wait_for_completion(&done);
>
> right here.
>
> > interrupted = ep->status < 0;
> > }
>
Thanks! This does seem to resolve the issue I'm seeing.
Are you planning to send in such a patch, or would it help if I sent it out?
thanks
-john
RE: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Heikki
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org On
> Behalf Of Heikki Krogerus
> Sent: Friday, February 1, 2019 2:48 AM
> To: Greg Kroah-Hartman ; Ajay Gupta
> ; Michael Hsu
> Cc: linux-usb@vger.kernel.org
> Subject: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
>
> With UCSI the alternate modes, just like everything else related to USB Type-C
> connectors, are handled in firmware.
> The operating system can see the status and is allowed to request certain
> things,
> for example entering and exiting the modes, but the support for alternate
> modes is very limited in UCSI. The feature is also optional, which means that
> even when the platform supports alternate modes, the operating system may
> not be even made aware of them.
>
> UCSI does not support direct VDM reading or writing.
> Instead, alternate modes can be entered and exited using a single custom
> command which takes also an optional SVID specific configuration value as
> parameter. That means every supported alternate mode has to be handled
> separately in UCSI driver.
>
> This commit does not include support for any specific alternate mode. The
> discovered alternate modes are now registered, but binding a driver to an
> alternate mode will not be possible until support for that alternate mode is
> added to the UCSI driver.
>
> Signed-off-by: Heikki Krogerus
> ---
> drivers/usb/typec/ucsi/trace.c | 12 ++ drivers/usb/typec/ucsi/trace.h |
> 26 +++
> drivers/usb/typec/ucsi/ucsi.c | 351 +++--
> drivers/usb/typec/ucsi/ucsi.h | 72 +++
> 4 files changed, 396 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/trace.c b/drivers/usb/typec/ucsi/trace.c
> index
> ffa3b4c3f338..1dabafb74320 100644
> --- a/drivers/usb/typec/ucsi/trace.c
> +++ b/drivers/usb/typec/ucsi/trace.c
> @@ -60,3 +60,15 @@ const char *ucsi_cci_str(u32 cci)
>
> return "";
> }
> +
> +static const char * const ucsi_recipient_strs[] = {
> + [UCSI_RECIPIENT_CON]= "port",
> + [UCSI_RECIPIENT_SOP]= "partner",
> + [UCSI_RECIPIENT_SOP_P] = "plug (prime)",
> + [UCSI_RECIPIENT_SOP_PP] = "plug (double prime)",
> +};
> +
> +const char *ucsi_recipient_str(u8 recipient) {
> + return ucsi_recipient_strs[recipient]; }
> diff --git a/drivers/usb/typec/ucsi/trace.h b/drivers/usb/typec/ucsi/trace.h
> index
> 5e2906df2db7..783ec9c72055 100644
> --- a/drivers/usb/typec/ucsi/trace.h
> +++ b/drivers/usb/typec/ucsi/trace.h
> @@ -7,6 +7,7 @@
> #define __UCSI_TRACE_H
>
> #include
> +#include
>
> const char *ucsi_cmd_str(u64 raw_cmd);
> const char *ucsi_ack_str(u8 ack);
> @@ -134,6 +135,31 @@ DEFINE_EVENT(ucsi_log_connector_status,
> ucsi_register_port,
> TP_ARGS(port, status)
> );
>
> +DECLARE_EVENT_CLASS(ucsi_log_register_altmode,
> + TP_PROTO(u8 recipient, struct typec_altmode *alt),
> + TP_ARGS(recipient, alt),
> + TP_STRUCT__entry(
> + __field(u8, recipient)
> + __field(u16, svid)
> + __field(u8, mode)
> + __field(u32, vdo)
> + ),
> + TP_fast_assign(
> + __entry->recipient = recipient;
> + __entry->svid = alt->svid;
> + __entry->mode = alt->mode;
> + __entry->vdo = alt->vdo;
> + ),
> + TP_printk("%s alt mode: svid %04x, mode %d vdo %x",
> + ucsi_recipient_str(__entry->recipient), __entry->svid,
> + __entry->mode, __entry->vdo)
> +);
> +
> +DEFINE_EVENT(ucsi_log_register_altmode, ucsi_register_altmode,
> + TP_PROTO(u8 recipient, struct typec_altmode *alt),
> + TP_ARGS(recipient, alt)
> +);
> +
> #endif /* __UCSI_TRACE_H */
>
> /* This part must be outside protection */ diff --git
> a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index
> 8d0a6fe748bd..5190f8dd4548 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -12,7 +12,7 @@
> #include
> #include
> #include
> -#include
> +#include
>
> #include "ucsi.h"
> #include "trace.h"
> @@ -45,22 +45,6 @@ enum ucsi_status {
> UCSI_ERROR,
> };
>
> -struct ucsi_connector {
> - int num;
> -
> - struct ucsi *ucsi;
> - struct work_struct work;
> - struct completion complete;
> -
> - struct typec_port *port;
> - struct typec_partner *partner;
> -
> - struct typec_capability typec_cap;
> -
> - struct ucsi_connector_status status;
> - struct ucsi_connector_capability cap;
> -};
> -
> struct ucsi {
> struct device *dev;
> struct ucsi_ppm *ppm;
> @@ -238,8 +222,201 @@ static int ucsi_run_command(struct ucsi *ucsi, struct
> ucsi_control *ctrl,
> return ret;
> }
>
> +int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
> + void *retval, size_t size)
> +{
> + int ret;
> +
> + mutex_lock(&ucsi->ppm_lock);
> + ret = ucsi_run_command(ucsi, ctr
RE: [PATCH 5/5] usb: typec: ucsi: Support for DisplayPort alt mode
Hi Heikki,
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org On
> Behalf Of Heikki Krogerus
> Sent: Friday, February 1, 2019 2:48 AM
> To: Greg Kroah-Hartman ; Ajay Gupta
> ; Michael Hsu
> Cc: linux-usb@vger.kernel.org
> Subject: [PATCH 5/5] usb: typec: ucsi: Support for DisplayPort alt mode
>
> This makes it possible to bind a driver to a DisplayPort alt mode adapter
> devices.
>
> The driver attempts to cope with the limitations of UCSI by "emulating"
> behaviour and attempting to guess things when ever possible in order to
> satisfy
> the requirements the standard DisplayPort alt mode driver has.
>
> Signed-off-by: Heikki Krogerus
> ---
> drivers/usb/typec/ucsi/Makefile | 15 +-
> drivers/usb/typec/ucsi/displayport.c | 301 +++
> drivers/usb/typec/ucsi/ucsi.c| 22 +-
> drivers/usb/typec/ucsi/ucsi.h| 21 ++
> 4 files changed, 351 insertions(+), 8 deletions(-) create mode 100644
> drivers/usb/typec/ucsi/displayport.c
>
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 2f4900b26210..b35e15a1f02c 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -1,12 +1,15 @@
> # SPDX-License-Identifier: GPL-2.0
> -CFLAGS_trace.o := -I$(src)
> +CFLAGS_trace.o := -I$(src)
>
> -obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
> +obj-$(CONFIG_TYPEC_UCSI) += typec_ucsi.o
>
> -typec_ucsi-y := ucsi.o
> +typec_ucsi-y := ucsi.o
>
> -typec_ucsi-$(CONFIG_TRACING) += trace.o
> +typec_ucsi-$(CONFIG_TRACING) += trace.o
>
> -obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
> +ifneq ($(CONFIG_TYPEC_DP_ALTMODE),)
> + typec_ucsi-y+= displayport.o
> +endif
>
> -obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
> +obj-$(CONFIG_UCSI_ACPI) += ucsi_acpi.o
> +obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/displayport.c
> b/drivers/usb/typec/ucsi/displayport.c
> new file mode 100644
> index ..3c5312cc7130
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/displayport.c
> @@ -0,0 +1,301 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI DisplayPort Alternate Mode Support
> + *
> + * Copyright (C) 2018, Intel Corporation
> + * Author: Heikki Krogerus */
> +
> +#include
> +#include
> +
> +#include "ucsi.h"
> +
> +#define UCSI_CMD_SET_NEW_CAM(_con_num_, _enter_, _cam_, _am_)
> \
> + (UCSI_SET_NEW_CAM | ((_con_num_) << 16) | ((_enter_) << 23) |
> \
> + ((_cam_) << 24) | ((u64)(_am_) << 32))
> +
> +struct ucsi_dp {
> + struct typec_displayport_data data;
> + struct ucsi_connector *con;
> + struct typec_altmode *alt;
> + struct work_struct work;
> + int offset;
> +
> + bool override;
> + bool initialized;
> +
> + u32 header;
> + u32 *vdo_data;
> + u8 vdo_size;
> +};
> +
> +/*
> + * Note. Alternate mode control is optional feature in UCSI. It means
> +that even
> + * if the system supports alternate modes, the OS may not be aware of them.
> + *
> + * In most cases however, the OS will be able to see the supported
> +alternate
> + * modes, but it may still not be able to configure them, not even
> +enter or exit
> + * them. That is because UCSI defines alt mode details and alt mode
> "overriding"
> + * as separate options.
> + *
> + * In case alt mode details are supported, but overriding is not, the
> +driver
> + * will still display the supported pin assignments and configuration,
> +but any
> + * changes the user attempts to do will lead into failure with return
> +value of
> + * -EOPNOTSUPP.
> + */
> +
> +static int ucsi_displayport_enter(struct typec_altmode *alt) {
> + struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
> +
> + mutex_lock(&dp->con->lock);
> +
> + if (!dp->override && dp->initialized) {
> + const struct typec_altmode *p =
> typec_altmode_get_partner(alt);
> +
> + dev_warn(&p->dev,
> + "firmware doesn't support alternate mode
> overriding\n");
> + mutex_unlock(&dp->con->lock);
> + return -EOPNOTSUPP;
> + }
> +
> + /*
> + * We can't send the New CAM command yet to the PPM as it needs the
> + * configuration value as well. Pretending that we have now entered the
> + * mode, and letting the alt mode driver continue.
> + */
> +
> + dp->header = VDO(USB_TYPEC_DP_SID, 1, CMD_ENTER_MODE);
> + dp->header |= VDO_OPOS(USB_TYPEC_DP_MODE);
> + dp->header |= VDO_CMDT(CMDT_RSP_ACK);
> +
> + dp->vdo_data = NULL;
> + dp->vdo_size = 1;
> +
> + schedule_work(&dp->work);
> +
> + mutex_unlock(&dp->con->lock);
> +
> + return 0;
> +}
> +
> +static int ucsi_displayport_exit(struct typec_altmode *alt) {
> + struct ucsi_dp *dp = typec_altmode_get_drvdata(alt);
RE: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate modes
Hi Heikki, > -Original Message- > From: Heikki Krogerus > Sent: Monday, February 4, 2019 4:10 AM > To: Michael Hsu > Cc: Greg Kroah-Hartman ; Ajay Gupta > ; linux-usb@vger.kernel.org > Subject: Re: [PATCH 4/5] usb: typec: ucsi: Preliminary support for alternate > modes > > Hi, > > On Fri, Feb 01, 2019 at 10:02:19PM +, Michael Hsu wrote: > > Hi Heikki, the use of "con->port_altmode[cur]->mode" (which is a > > 1-based index, not a 32-bit mode VDO) can cause incorrect matches if > > the GET_ALTERNATE_MODES returns different ordering for > > recipient=connector and recipient=sop for a particular svid. > > > > For example, UCSI command GET_ALTERNATE_MODES with > recipient=connector returns > > [0] SVID=0xff01, ModeVDO=0x0405 (Mode = 1) > > [1] SVID=0xff01, ModeVDO=0x0805 (Mode = 2) And UCSI command > > GET_ALTERNATE_MODES with recipient=sop returns > > [0] SVID=0xff01, ModeVDO=0x0c05 (Mode = 1) > > > > Then when DP alternate mode with pin D is active, GET_CURRENT_CAM > > returns index 1 (connector alternate mode = 1, i.e. SVID=0xff01, > > Mode=2, ModeVDO=0x0805). > > But, the function will be unable to match it with partner_alt_mode > > corresponding to (SVID=0xff01,Mode=1). > > > > Can you compare against 32-bit VDO instead of ->mode? Also, use '&' > > bitwise AND operator when masking the partner's mode VDO (0x0c05) > > against the connector's mode VDO (0x405 or 0x805) to determine it > > there is an alternate mode match. > > No top-posting! From now on inline your comments in your replies. The > "process" guide in kernel should provide more information for you: > https://www.kernel.org/doc/html/latest/process/2.Process.html#mailing-lists > OK, got it... answering inline. > Your PPM is reporting a separate mode for every pin-assignment it supports. It > really should _not_ do that! You need to be able to get the capabilities for > DP > alt mode with GET_ALTERNATE_MODE command in the format that the DP alt > mode spec defines, also with the connector. > You are not getting them like that. Now for example in both modes your > connector can only act as DisplayPort sink (i.e. a display) which I'm pretty > sure > is not the case. > > With the current version of the DP alt mode spec we do not ever need more > than one mode for DP. That mode needs to show all the supported pin > assignments the device, partner or connector, supports. That is exactly how > all > the other UCSI PPMs work currently. For example the PPM on Dell XPS 13 9380 > that I use for testing gives only one mode for the connector with SVID ff01. > The > mode VDO (or MID in UCSI lingo) has the value 0x1c46: > > SVID=0xff01, VDO=0x1c46 > > From that you can clearly see that the connector can only act as the > DisplayPort > source, and that it support pin assignments C, D and E. > > So in practice you have a firmware bug. I understand why the PPM was made > that way. That "hack" allows the PPM to workaround a limitation in UCSI where > we in practice can't tell which configuration is in use at any given time. > Unfortunately it can not be done like that. It leaves us with custom VDO > values > that we would have to be interpreted separately, that only your PPM supports. > I still think it complies with the letter and spirit of the UCSI spec... There was nothing in the document to suggest that this method of implementing alt modes (i.e., assigning one connector alt mode index per DP pin configuration) is not allowed. Actually, there's also a (great) benefit with this method, the UCSI GET_SUPPORT_CAM can now be used to determine which DP pin assignments are allowed - dynamically. For example, sometimes due to another (non-DisplayPort) alt mode being active, the pins used for DP pin C might not be available, but DP pin D is still allowed. So, a call to UCSI GET_SUPPORTED_CAM would indicate that the pin C DP alt mode was not supported, but the pin D DP alt mode was currently allowed. The drawback which you mentioned "custom VDO values" is not accurate. The GET_ALTERNATE_MODES commands still returns DP-compliant mode VDOs - i.e., bits 15-8 still indicate the DP pin configurations allowed when entering that DP-compatible alternate mode. My original suggestion was to use a bitwise AND mask to make the correlation between the connector's alt mode and the partner's alt mode. This has the benefit of (1) working with PPMs which report a single connector alternate mode for all the DP pin assignments (2) working with PPMs which report separate connector alternate modes for each DP pin assignment If we use your original patch which does a simple mode index equality comparison (instead of a bitwise AND mask), it cannot handle case (2) above. > Please see if you can get the firmware (UCSI PPM) fixed. It really needs to > expose only one mode for DisplayPort alt mode in the connector, and the VDO > in it should be in the same form that it could be send to the partner, i.e.
[PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY
Add a quirk to reset xHCI port PHY on port disconnect event.
Stingray USB HS PHY has an issue, that USB High Speed device detected
at Full Speed after the same port has connected to Full speed device.
This problem can be resolved with that port PHY reset on disconnect.
Signed-off-by: Srinath Mannam
Reviewed-by: Ray Jui
---
drivers/usb/core/hcd.c | 6 ++
drivers/usb/core/phy.c | 21 +
drivers/usb/core/phy.h | 1 +
drivers/usb/host/xhci-plat.c | 3 +++
drivers/usb/host/xhci-ring.c | 9 ++---
drivers/usb/host/xhci.h | 1 +
include/linux/usb/hcd.h | 1 +
7 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 015b126..e2b87a6 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd,
int port1)
return hcd->driver->find_raw_port_number(hcd, port1);
}
+int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
+{
+ return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
+}
+EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
+
static int usb_hcd_request_irqs(struct usb_hcd *hcd,
unsigned int irqnum, unsigned long irqflags)
{
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 38b2c77..c64767d 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub
*phy_roothub)
}
EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
+{
+ struct usb_phy_roothub *roothub_entry;
+ struct list_head *head;
+ int i = 0;
+
+ if (!phy_roothub)
+ return -EINVAL;
+
+ head = &phy_roothub->list;
+
+ list_for_each_entry(roothub_entry, head, list) {
+ if (i == port)
+ return phy_reset(roothub_entry->phy);
+ i++;
+ }
+
+ return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(usb_phy_roothub_port_reset);
+
int usb_phy_roothub_suspend(struct device *controller_dev,
struct usb_phy_roothub *phy_roothub)
{
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 88a3c03..e8be444 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -18,6 +18,7 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
+int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port);
int usb_phy_roothub_suspend(struct device *controller_dev,
struct usb_phy_roothub *phy_roothub);
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ef09cb0..5a3b486 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -289,6 +289,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
xhci->quirks |= XHCI_BROKEN_PORT_PED;
+ if (device_property_read_bool(tmpdev, "usb-phy-port-reset"))
+ xhci->quirks |= XHCI_RESET_PHY_ON_DISCONNECT;
+
device_property_read_u32(tmpdev, "imod-interval-ns",
&xhci->imod_interval);
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c..2dc3116 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1685,9 +1685,12 @@ static void handle_port_status(struct xhci_hcd *xhci,
if (hcd->speed < HCD_USB3) {
xhci_test_and_clear_bit(xhci, port, PORT_PLC);
- if ((xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT) &&
- (portsc & PORT_CSC) && !(portsc & PORT_CONNECT))
- xhci_cavium_reset_phy_quirk(xhci);
+ if ((portsc & PORT_CSC) && !(portsc & PORT_CONNECT)) {
+ if (xhci->quirks & XHCI_RESET_PLL_ON_DISCONNECT)
+ xhci_cavium_reset_phy_quirk(xhci);
+ else if (xhci->quirks & XHCI_RESET_PHY_ON_DISCONNECT)
+ usb_hcd_phy_port_reset(hcd, port_id - 1);
+ }
}
cleanup:
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36..530c5ff 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1846,6 +1846,7 @@ struct xhci_hcd {
#define XHCI_DEFAULT_PM_RUNTIME_ALLOW BIT_ULL(33)
#define XHCI_RESET_PLL_ON_DISCONNECT BIT_ULL(34)
#define XHCI_SNPS_BROKEN_SUSPENDBIT_ULL(35)
+#define XHCI_RESET_PHY_ON_DISCONNECT BIT_ULL(36)
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
diff --git a/include/linux/usb/hcd.h b/include/linux
[PATCH 0/2] Reset xHCI port PHY on disconnect
This patch set adds a quirk in xHCI driver to reset PHY of xHCI port on its disconnect event. This patch set is based on Linux-5.0-rc2. Srinath Mannam (2): dt-bindings: usb-xhci: Add usb-phy-port-reset property drivers: xhci: Add quirk to reset xHCI port PHY Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 + drivers/usb/core/hcd.c | 6 ++ drivers/usb/core/phy.c | 21 + drivers/usb/core/phy.h | 1 + drivers/usb/host/xhci-plat.c | 3 +++ drivers/usb/host/xhci-ring.c | 9 ++--- drivers/usb/host/xhci.h| 1 + include/linux/usb/hcd.h| 1 + 8 files changed, 40 insertions(+), 3 deletions(-) -- 2.7.4
[PATCH 1/2] dt-bindings: usb-xhci: Add usb-phy-port-reset property
Add usb-phy-port-reset optional property to set quirk in xhci platform driver which forces USB port PHY reset on port disconnect event. Signed-off-by: Srinath Mannam Reviewed-by: Ray Jui --- Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt index fea8b15..ecbdb15 100644 --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt @@ -40,6 +40,7 @@ Optional properties: - usb3-lpm-capable: determines if platform is USB3 LPM capable - quirk-broken-port-ped: set if the controller has broken port disable mechanism - imod-interval-ns: default interrupt moderation interval is 5000ns + - usb-phy-port-reset: set this to do USB PORT PHY reset while disconnect - phys : see usb-hcd.txt in the current directory additionally the properties from usb-hcd.txt (in the current directory) are -- 2.7.4
