[PATCH v15 2/2] usb: typec: ucsi: add support for Cypress CCGx
Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
over I2C interface.
This UCSI I2C driver uses I2C bus driver interface for communicating
with Type-C controller.
Signed-off-by: Ajay Gupta
---
Changes from v1 -> v2
Fixed identation in drivers/usb/typec/ucsi/Kconfig
Changes from v2 -> v3
Fixed most of comments from Heikki
Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
Changes from v3 -> v4
Fixed comments from Andy
Changes from v4 -> v5
Fixed comments from Andy
Changes from v5 -> v6
Fixed review comments from Greg
Changes from v6 -> v7
None
Changes from v7 -> v8
Fixed review comments from Peter
- Removed empty STOP message
- Using stack memory for i2c_transfer()
Changes from v8 -> v9
None
Changes from v9 -> v10
Fixed review comments from Peter
- Use UCSI macros
- Cleanups
Changes from v10 -> v11
Fixed review comments from Peter
- Combined two writes into one
- Used offsetof() for ucsi_data struct
Changes from v11 -> v12
- some cleanup
Changes from v12 -> v13
- changed "u8 buf[1]" -> "u8 data"
Changes from v13 -> v14
- Fixed commend from Heikki and Andy
- Added i2c adapters quirks check in ccg_read
- Removed "irq" field from struct ucsi_ccg
- Reorganize do {} while (); loop
Changes from v14 -> v15
- Fix unnecessary check for "data" in while () condition
drivers/usb/typec/ucsi/Kconfig| 10 +
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_ccg.c | 307 ++
3 files changed, 319 insertions(+)
create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index e36d6c73c4a4..78118883f96c 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -23,6 +23,16 @@ config TYPEC_UCSI
if TYPEC_UCSI
+config UCSI_CCG
+ tristate "UCSI Interface Driver for Cypress CCGx"
+ depends on I2C
+ help
+ This driver enables UCSI support on platforms that expose a
+ Cypress CCGx Type-C controller over I2C interface.
+
+ To compile the driver as a module, choose M here: the module will be
+ called ucsi_ccg.
+
config UCSI_ACPI
tristate "UCSI ACPI Interface Driver"
depends on ACPI
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 7afbea512207..2f4900b26210 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -8,3 +8,5 @@ typec_ucsi-y:= ucsi.o
typec_ucsi-$(CONFIG_TRACING) += trace.o
obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+
+obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
new file mode 100644
index ..de8a43bdff68
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for Cypress CCGx Type-C controller
+ *
+ * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta
+ *
+ * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include "ucsi.h"
+
+struct ucsi_ccg {
+ struct device *dev;
+ struct ucsi *ucsi;
+ struct ucsi_ppm ppm;
+ struct i2c_client *client;
+};
+
+#define CCGX_RAB_INTR_REG 0x06
+#define CCGX_RAB_UCSI_CONTROL 0x39
+#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
+
+static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+ struct i2c_client *client = uc->client;
+ const struct i2c_adapter_quirks *quirks = client->adapter->quirks;
+ unsigned char buf[2];
+ struct i2c_msg msgs[] = {
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .len= sizeof(buf),
+ .buf= buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .buf= data,
+ },
+ };
+ u32 rlen, rem_len = len, max_read_len = len;
+ int status;
+
+ /* check any max_read_len limitation on i2c adapter */
+ if (quirks && quirks->max_read_len)
+ max_read_len = quirks->max_read_len;
+
+ while (rem_len > 0) {
+ msgs[1].buf = [len - rem_len];
+ rlen = min_t(u16, rem_len, max_read_len);
+ msgs[1].len = rlen;
+ put_unaligned_le16(rab, buf);
+ status = i2c_transfer(client->adapter, msgs,
[PATCH v15 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C controller which can be accessed over I2C. This driver adds I2C bus driver to communicate with Type-C controller. I2C client driver will be part of USB Type-C UCSI driver. Signed-off-by: Ajay Gupta --- Changes from v1 -> v2 None Changes from v2 -> v3 Fixed review comments from Andy and Thierry Rename i2c-gpu.c -> i2c-nvidia-gpu.c Changes from v3 -> v4 Fixed review comments from Andy Changes from v4 -> v5 Fixed review comments from Andy Changes from v5 -> v6 None Changes from v6 -> v7 -> v8 Fixed review comments from Peter - Add implicit STOP for last write message - Add i2c_adapter_quirks with max_read_len and I2C_AQ_COMB flags Changes from v8 -> v9 Fixed review comments from Peter - Drop do_start flag - Use i2c_8bit_addr_from_msg() Changes from v9 -> v10 Fixed review comments from Peter - Dropped I2C_FUNC_SMBUS_EMUL - Dropped local mutex Changes from v10 -> v11 Fixed review comments from Peter - Moved stop in master_xfer at end of message - Change i2c_read without STOP - Dropped I2C_AC_COMB* flags Changes from v11 -> v12 Fixed review comments from Peter - Removed clearing of empty bits - Fix master_xfer for correct stop use Changes from v12 -> v13 Fixed review comments from Peter - Added comments on 4 byte read limitation - Added I2C_AC_COMB* flags Changes from v13 -> v14 -> v15 None Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nvidia-gpu.c | 368 5 files changed, 403 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu new file mode 100644 index ..31884d2b2eb5 --- /dev/null +++ b/Documentation/i2c/busses/i2c-nvidia-gpu @@ -0,0 +1,18 @@ +Kernel driver i2c-nvidia-gpu + +Datasheet: not publicly available. + +Authors: + Ajay Gupta + +Description +--- + +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing +and later GPUs and it is used to communicate with Type-C controller on GPUs. + +If your 'lspci -v' listing shows something like the following, + +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1) + +then this driver should support the I2C controller of your GPU. diff --git a/MAINTAINERS b/MAINTAINERS index bd702ad56c7f..5885a0931de9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6843,6 +6843,13 @@ L: linux-a...@vger.kernel.org S: Maintained F: drivers/i2c/i2c-core-acpi.c +I2C CONTROLLER DRIVER FOR NVIDIA GPU +M: Ajay Gupta +L: linux-...@vger.kernel.org +S: Maintained +F: Documentation/i2c/busses/i2c-nvidia-gpu +F: drivers/i2c/busses/i2c-nvidia-gpu.c + I2C MUXES M: Peter Rosin L: linux-...@vger.kernel.org diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..eed827b44068 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985 This driver can also be built as a module. If so, the module will be called i2c-nforce2-s4985. +config I2C_NVIDIA_GPU + tristate "NVIDIA GPU I2C controller" + depends on PCI + help + If you say yes to this option, support will be included for the + NVIDIA GPU I2C controller which is used to communicate with the GPU's + Type-C controller. This driver can also be built as a module called + i2c-nvidia-gpu. + config I2C_SIS5595 tristate "SiS 5595" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 18b26af82b1c..d499813df038 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)+= i2c-sibyte.o obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o obj-$(CONFIG_I2C_FSI) += i2c-fsi.o +obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c new file mode 100644 index ..744f5e42636b --- /dev/null +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c @@ -0,0 +1,368 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Nvidia GPU I2C controller Driver + * + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved. + * Author: Ajay Gupta + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include
[PATCH v15 0/2] Add support for USB Type-C interface on latest NVIDIA GPU
Hi Heikki and Wolfram, These two changes add support for USB Type-C interface on latest NVIDIA GPU card. The Type-C controller used is Cypress CCGx and is over I2C interface. I2C host controller has known limitation of sending STOP after every read. Since each read can be of 4 byte maximum length so there is a limit of 4 byte read. This is mentioned in adapter quirks as "max_read_len = 4" I2C host controller is mainly used for "write-then-read" or "write" messages so added the flag I2C_AQ_COMB_WRITE_THEN_READ in adapter quirks. PATCH[2/2] on ucsi driver now have added logic to check i2c adapter quirks and issues i2c read transfer based on max_read_len quirk settings. This will make sure the read limitation is not affecting I2C host which do not have such limitation. I think the patches should through usb tree because the main functionality is usb Type-C. Thanks Ajay Ajay Gupta (2): i2c: buses: add i2c bus driver for NVIDIA GPU usb: typec: ucsi: add support for Cypress CCGx Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nvidia-gpu.c | 368 drivers/usb/typec/ucsi/Kconfig | 10 + drivers/usb/typec/ucsi/Makefile | 2 + drivers/usb/typec/ucsi/ucsi_ccg.c | 307 8 files changed, 722 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c -- 2.17.1
RE: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Peter and Andy,
> Shouldn't you return -ETIMEDOUT if count == 0?
> >>> Yes. Good catch. Does the below fix looks ok?
> >>>
> >>> do {
> >>> status = ccg_write(uc, CCGX_RAB_INTR_REG, ,
> sizeof(data));
> >>> if (status < 0)
> >>> return status;
> >>>
> >>> usleep_range(1, 11000);
> >>>
> >>> status = ccg_read(uc, CCGX_RAB_INTR_REG, ,
> >>> sizeof(data));
> >>> if (status < 0)
> >>> return status;
> >>>
> >>> if (!data)
> >>> return 0;
> >>> } while (data && count--);
> >>
> >> Doesn't that condition break out of the loop immediately?
> > How? I didn't get your point? We want to break out when data is zero
> > (interrupt status cleared).
>
> The statement
>
> if (!data)
> return 0;
>
> ensures that 'data' is non-zero when the loop continues, so checking that
> 'data' is non-zero in the while loop test is pointless.
>
> >>> Ah, I see, but why you not reorganize it to put this into do-while loop?
> > We actually need to check data after reading it so will reorganize
> accordingly.
Sorry, my bad, will revise.
Thanks
Ajay
--nvpublic
> > do {
> > read
> > check for data and break out if (!data)
> > write
> > sleep
> > } while (--count);
>
> Here, you have fixed the "issue" (but it doesn't match v14).
>
> Cheers,
> Peter
Re: [PATCH v2] HID: hiddev: fix potential Spectre v1
On Fri, 19 Oct 2018, Breno Leitao wrote:
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
>
> This field is used as an array index by the hiddev_ioctl_usage() function,
> when 'cmd' is either HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or
> HIDIOCSUSAGES.
>
> For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
> field->maxusage and then used as an index to dereference field->usage
> array. The same thing happens to the cmd == HIDIOC{G,S}USAGES cases, where
> uref->usage_index is checked against an array maximum value and then it is
> used as an index in an array.
>
> This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
> traditional Spectre V1 first load:
>
> copy_from_user(uref, user_arg, sizeof(*uref))
> if (uref->usage_index >= field->maxusage)
> goto inval;
> i = field->usage[uref->usage_index].collection_index;
> return i;
>
> This patch fixes this by sanitizing field uref->usage_index before using it
> to index field->usage (HIDIOCGCOLLECTIONINDEX) or field->value in
> HIDIOC{G,S}USAGES arrays, thus, avoiding speculation in the first load.
>
> Signed-off-by: Breno Leitao
> Cc:
Applied, thanks.
--
Jiri Kosina
SUSE Labs
[RESEND PATCH v2] usb: dwc2: disable power_down on rockchip for regression
>From 04fbf78e4e569bf872f1ffcb0a6f9b89569dc913 Mon Sep 17 00:00:00 2001 From: Hal Emmerich Date: Thu, 19 Jul 2018 21:48:08 -0500 Subject: [PATCH] usb: dwc2: disable power_down on rockchip devices The bug would let the usb controller enter partial power down, which was formally known as hibernate, upon boot if nothing was plugged in to the port. Partial power down couldn't be exited properly, so any usb devices plugged in after boot would not be usable. Before the name change, params.hibernation was false by default, so _dwc2_hcd_suspend() would skip entering hibernation. With the rename, _dwc2_hcd_suspend() was changed to use params.power_down to decide whether or not to enter partial power down. Since params.power_down is non-zero by default, it needs to be set to 0 for rockchip devices to restore functionality. This bug was reported in the linux-usb thread: REGRESSION: usb: dwc2: USB device not seen after boot The commit that caused this regression is: 6d23ee9caa6790aea047f9aca7f3c03cb8d96eb6 Signed-off-by: Hal Emmerich Acked-by: Minas Harutyunyan --- drivers/usb/dwc2/params.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index bf7052e037d6..09292dc977e4 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -81,6 +81,7 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg) p->host_perio_tx_fifo_size = 256; p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT; + p->power_down = 0; } static void dwc2_set_ltq_params(struct dwc2_hsotg *hsotg) -- 2.11.0
Re: EPROTO when USB 3 GbE adapters are under load
On Fri, 26 Oct 2018, Mathias Nyman wrote: > On 25.10.2018 20:28, Alan Stern wrote: > > On Thu, 25 Oct 2018, Mathias Nyman wrote: > > > >> On 25.10.2018 12:52, Hao Wei Tee wrote: > >>> On 25/10/18 4:45 PM, Mathias Nyman wrote: > Reproducing the issue with a recent kernel with xhci traces enabled > should show the reason for EPROTO error. > > Add xhci traces before triggering the issue with: > > 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 issue is triggered save and send the trace at > /sys/kernel/debug/tracing/trace > Note that it might be huge > >>> > >>> Thanks for the suggestion. > >>> > >>> Here[1] is (part of) the trace starting about 250 lines before the EPROTO > >>> happens. > >>> > >>> [1]: > >>> https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace > >>> > >>> The first error happens at line 243 (timestamp 8144.248398) coinciding > >>> with the start of errors spewed into dmesg: > >>> > >>> [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 > >> > >> Thanks, > >> xHC controller reports that there was a transaction error on one of the > >> bulk TRBs. > >> > >> The transaction error causes the endpoint to halt (host side halt only). > >> Xhci driver resets the host side endpoint to recover from the halt, > >> then returns the broken URB (TRB) with -EPROTO status, and then moves past > >> this TRB. > > > > The host side of the endpoint should remain stopped until after the > > URB's completion routine has had a chance to carry out error recovery. > > Doesn't this imply the xHCI driver shouldn't reset the host-side > > endpoint until after the giveback call returns? > > True, on xhci side we could probably reset the endpoint, and even move the > dequeue pointer to the next TRB, but make sure the endpoint is not restarted > yet. > > The URB with -EPROTO status is given back in interrupt context, so this might > limit > a bit what the higher-layer drivers can do in giveback. One thing they can do is unlink any URBs still remaining in the endpoint's queue, thus preventing any confusion from stale data when the endpoint restarts. It's okay to call usb_unlink_urb() in interrupt context. > Now thinking about it, xhci driver calls the URB giveback in the same > "Transaction error" > interrupt handler, after first queuing areset endpoint and a set TR Deq > pointer command. > The endpoint is only restarted after those commands finish, in the command > completion interrupt > handler. > > So in that sense the endpoint shouldn't be restarted until the next interrupt > is handled, > which shouldn't be possible before the URB giveback call returned in the > previous interrupt handler. > > Well, at least not as long as we are in hard interrupt. > > I think I need to dig a bit more into this. > > > > >> Interesting thing here is that each TRB in the queue after the transaction > >> error > >> also triggers a transaction error. > >> > >> This might be a data toggle/sequence number sync issue. > > > > It's more likely to be a problem on the device side. Data toggle or > > sequence number issues tend to be self-repairing (albeit with some data > > loss) after a little while. > > Ok, thanks, not spending too much time looking into that then. Important point: The device's problem might be caused by the kernel sending it a command it can't handle. So maybe the way to fix the problem may be to change the upper-layer driver; this happens sometimes. Other times it really is just a bug in the device. > >> The host side endpoint reset clears the host side sequence number, > >> and host expects device side endpoint to be reset and sequence to be > >> cleared as well > >> as a result of returning -EPROTO. > >> If I remember correctly xhci driver does not wait for device side endpoint > >> to be reset, > >> so if there are TRBs in the queue they will be transferred, with a > >> cleared sequence number > >> out of sync with the device side. > > > > That's why it's important to wait until after the higher-layer driver > > has had a chance to unlink the URBs that may be in the endpoint queue. > > The driver may even want to reset the device. > > Would it make sense to prevent endpoint from running until usb core calls > hcd->driver->endpoint_reset? > That is for halted endpoints, that returned URB with -EPROTO status. The HCD shouldn't worry about that. The higher-layer driver is responsible for fixing the error that caused the endpoint to halt, unlinking any remaining URBs, and clearing the halt. Alan Stern
Re: EPROTO when USB 3 GbE adapters are under load
On 25.10.2018 20:28, Alan Stern wrote: On Thu, 25 Oct 2018, Mathias Nyman wrote: On 25.10.2018 12:52, Hao Wei Tee wrote: On 25/10/18 4:45 PM, Mathias Nyman wrote: Reproducing the issue with a recent kernel with xhci traces enabled should show the reason for EPROTO error. Add xhci traces before triggering the issue with: 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 issue is triggered save and send the trace at /sys/kernel/debug/tracing/trace Note that it might be huge Thanks for the suggestion. Here[1] is (part of) the trace starting about 250 lines before the EPROTO happens. [1]: https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace The first error happens at line 243 (timestamp 8144.248398) coinciding with the start of errors spewed into dmesg: [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 Thanks, xHC controller reports that there was a transaction error on one of the bulk TRBs. The transaction error causes the endpoint to halt (host side halt only). Xhci driver resets the host side endpoint to recover from the halt, then returns the broken URB (TRB) with -EPROTO status, and then moves past this TRB. The host side of the endpoint should remain stopped until after the URB's completion routine has had a chance to carry out error recovery. Doesn't this imply the xHCI driver shouldn't reset the host-side endpoint until after the giveback call returns? True, on xhci side we could probably reset the endpoint, and even move the dequeue pointer to the next TRB, but make sure the endpoint is not restarted yet. The URB with -EPROTO status is given back in interrupt context, so this might limit a bit what the higher-layer drivers can do in giveback. Now thinking about it, xhci driver calls the URB giveback in the same "Transaction error" interrupt handler, after first queuing areset endpoint and a set TR Deq pointer command. The endpoint is only restarted after those commands finish, in the command completion interrupt handler. So in that sense the endpoint shouldn't be restarted until the next interrupt is handled, which shouldn't be possible before the URB giveback call returned in the previous interrupt handler. Well, at least not as long as we are in hard interrupt. I think I need to dig a bit more into this. Interesting thing here is that each TRB in the queue after the transaction error also triggers a transaction error. This might be a data toggle/sequence number sync issue. It's more likely to be a problem on the device side. Data toggle or sequence number issues tend to be self-repairing (albeit with some data loss) after a little while. Ok, thanks, not spending too much time looking into that then. The host side endpoint reset clears the host side sequence number, and host expects device side endpoint to be reset and sequence to be cleared as well as a result of returning -EPROTO. If I remember correctly xhci driver does not wait for device side endpoint to be reset, so if there are TRBs in the queue they will be transferred, with a cleared sequence number out of sync with the device side. That's why it's important to wait until after the higher-layer driver has had a chance to unlink the URBs that may be in the endpoint queue. The driver may even want to reset the device. Would it make sense to prevent endpoint from running until usb core calls hcd->driver->endpoint_reset? That is for halted endpoints, that returned URB with -EPROTO status. -Mathias
Re: [PATCH v14 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Thu, Oct 25, 2018 at 03:33:53PM -0700, aj...@nvidia.com wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
>
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> + /*
> + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi control
> + * register write will push response which must be cleared.
> + */
> + do {
> + status = ccg_read(uc, CCGX_RAB_INTR_REG, , sizeof(data));
> + if (status < 0)
> + return status;
> +
> + if (!data)
> + return 0;
> +
> + status = ccg_write(uc, CCGX_RAB_INTR_REG, , sizeof(data));
> + if (status < 0)
> + return status;
> +
> + usleep_range(1, 11000);
> + } while (data && --count);
I don't see any point to check data here. How can it be different from the
check above?
> + return -ETIMEDOUT;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Thu, Oct 25, 2018 at 09:55:47PM +, Ajay Gupta wrote:
> Hi Heikki and Andy
> [...]
> > > > Shouldn't you return -ETIMEDOUT if count == 0?
> > > Yes. Good catch. Does the below fix looks ok?
> > >
> > > do {
> > > status = ccg_write(uc, CCGX_RAB_INTR_REG, ,
> > > sizeof(data));
> > > if (status < 0)
> > > return status;
> > >
> > > usleep_range(1, 11000);
> > >
> > > status = ccg_read(uc, CCGX_RAB_INTR_REG, ,
> > > sizeof(data));
> > > if (status < 0)
> > > return status;
> > >
> > > if (!data)
> > > return 0;
> > > } while (data && count--);
> >
> > Doesn't that condition break out of the loop immediately?
> How? I didn't get your point? We want to break out when data is
> zero (interrupt status cleared).
Sorry Ajay. My brain interpreted that "while" as an "if" statement :-)
thanks,
--
heikki
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
On 2018-10-25 23:55, Ajay Gupta wrote:
> Hi Heikki and Andy
> [...]
Shouldn't you return -ETIMEDOUT if count == 0?
>>> Yes. Good catch. Does the below fix looks ok?
>>>
>>> do {
>>> status = ccg_write(uc, CCGX_RAB_INTR_REG, ,
>>> sizeof(data));
>>> if (status < 0)
>>> return status;
>>>
>>> usleep_range(1, 11000);
>>>
>>> status = ccg_read(uc, CCGX_RAB_INTR_REG, ,
>>> sizeof(data));
>>> if (status < 0)
>>> return status;
>>>
>>> if (!data)
>>> return 0;
>>> } while (data && count--);
>>
>> Doesn't that condition break out of the loop immediately?
> How? I didn't get your point? We want to break out when data is
> zero (interrupt status cleared).
The statement
if (!data)
return 0;
ensures that 'data' is non-zero when the loop continues, so checking
that 'data' is non-zero in the while loop test is pointless.
>>> Ah, I see, but why you not reorganize it to put this into do-while loop?
> We actually need to check data after reading it so will reorganize
> accordingly.
> do {
> read
> check for data and break out if (!data)
> write
> sleep
> } while (--count);
Here, you have fixed the "issue" (but it doesn't match v14).
Cheers,
Peter
