RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
Hi, Balbi, > -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Saturday, December 17, 2016 1:02 AM > To: Jerry Huang ; gre...@linuxfoundation.org > Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Rajesh Bhagat > > Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type > > > Hi, > > Jerry Huang writes: > > Hi, Balbi, > >> -Original Message- > >> From: Felipe Balbi [mailto:ba...@kernel.org] > >> Sent: Friday, December 16, 2016 7:44 PM > >> To: Jerry Huang ; gre...@linuxfoundation.org > >> Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Rajesh > >> Bhagat > >> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst > >> type > >> > >> > >> Hi, > >> > >> Jerry Huang writes: > >> >> there's no need for that. This patch is in good format. I do have > >> >> a question, > >> >> however: how do you know this will work for all users? Burst size > >> >> is a function of how wide the interconnect where dwc3 is attached to, > is. > >> > So I need to generate one new property in usb node to identify my > >> platform? > >> > >> Well, we probably need a property to be passed, yes. But let's go > >> through it all first :-) > > > > I think "snps,quirk-frame-length-adjustment" is one good reference, > > which can pass the required value to driver from DTS file. > > that's not for burst increment, though. I created one new property " snps,incr-burst-type-adjustment = , " to identify it from usb node, and will send out the v3 patch. > >> >> You could very well be degrading performance for some users here. > >> >> Can you send me the result of the following commands *without* > >> >> this patch > >> applied? > >> >> > >> >> # mkdir -p /d > >> >> # mount -t debugfs none /d > >> >> # cat /d/*dwc3*/regdump > >> >> > >> > Below is the regdump: > >> > root@ls1043ardb:/d/300.usb3# cat regdump > >> > GSBUSCFG0 = 0x00100080 > >> > >> so you already have INCR256 here. There's one note in the databook > >> which just caught my attention. It states the following: > >> > >>"Undefined burst length has priority over all other burst lenghts." > >> > >> This means that setting both INCR16 and undefined INCR is unnecessary. > > When bit0 = 1 (Undefined Length INCR Burst Type Enable), which means: > > 1: INCR (undefined length) burst mode > > - AHB configurations: HBURST uses SINGLE or INCR of any length less > > than or equal to the largest-enabled burst length of > > INCR4/8/16/32/64/128/256. > > - AXI configurations: ARLEN/AWLEN uses any length less than or equal > > to the largest-enabled burst length of INCR4/8/16/32/64/128/256. > > interesting, it doesn't describe what happens to OCP or PCI. Yes, just mention AHB and AXI from DWC superspeed USB3.0 controller databook (Version 2.50a, November 2012, maybe it is too old version for this IP). > > So, after enable undefined length INCR burst and enable INCR16, > > controller will use less than or equal to 16byte. > > > >> Only Undefined INCR will be taken into consideration. Can you check > >> with your HW engineers what's the largest burst the interconnect is > >> supposed to support? > > I will check it with IP designer. > > cool, thanks :-) Have confirmed with hardware engineer, the maximum INCR burst size of NXP platform is 16 byte > >> > GSBUSCFG1 = 0x0700 > >> > >> 8 AXI pipelined requests > >> > >> > GSNPSID = 0x5533280a > >> > >> 2.80a cool :-) > >> > >> I'll check these settings on my platform as well and see if there's > >> any setting which would improve transfer speed. This is a very good > >> idea, btw, but we need to be careful about how to play with it. > >> > >> -- > >> balbi > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-usb" > > in the body of a message to majord...@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
Hi, Jerry Huang writes: >> >> Jerry Huang writes: >> >> >> there's no need for that. This patch is in good format. I do have >> >> >> a question, >> >> >> however: how do you know this will work for all users? Burst size >> >> >> is a function of how wide the interconnect where dwc3 is attached to, >> is. >> >> > So I need to generate one new property in usb node to identify my >> >> platform? >> >> >> >> Well, we probably need a property to be passed, yes. But let's go >> >> through it all first :-) >> > >> > I think "snps,quirk-frame-length-adjustment" is one good reference, >> > which can pass the required value to driver from DTS file. >> >> that's not for burst increment, though. > > I created one new property " snps,incr-burst-type-adjustment = , > " to identify it from usb node, and will send out the v3 patch. okay, don't forget to Cc devicet...@vger.kernel.org. Let's see what those guys say. >> > So, after enable undefined length INCR burst and enable INCR16, >> > controller will use less than or equal to 16byte. >> > >> >> Only Undefined INCR will be taken into consideration. Can you check >> >> with your HW engineers what's the largest burst the interconnect is >> >> supposed to support? >> > I will check it with IP designer. >> >> cool, thanks :-) > > Have confirmed with hardware engineer, the maximum INCR burst size of > NXP platform is 16 byte good, thanks for that. -- balbi signature.asc Description: PGP signature
[PATCH v3 3/3] USB3/DWC3: Enable undefined length INCR burst type
Enable the undefined length INCR burst type and set INCRx.
Different platform may has the different burst size type.
In order to get best performance, we need to tune the burst size to
one special value, instead of the default value.
Signed-off-by: Changming Huang
Signed-off-by: Rajesh Bhagat
---
Changes in v3:
- add new property for INCR burst in usb node to reset GSBUSCFG0.
Changes in v2:
- split patch
- create one new function to handle soc bus configuration register.
drivers/usb/dwc3/core.c | 51 +++
drivers/usb/dwc3/core.h |7 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..404d7e9 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -650,6 +650,55 @@ static void dwc3_core_setup_global_control(struct dwc3
*dwc)
dwc3_writel(dwc->regs, DWC3_GCTL, reg);
}
+/* set global soc bus configuration registers */
+static void dwc3_set_soc_bus_cfg(struct dwc3 *dwc)
+{
+ struct device *dev = dwc->dev;
+ u32 cfg;
+ int ret;
+
+ cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
+
+ /* Get INCR burst type, if return !NULL, not to change this type */
+ ret = device_property_read_u32_array(dev,
+ "snps,incr-burst-type-adjustment",
+ dwc->incr_burst_type, 2);
+ if (!ret) {
+ /* Enable Undefined Length INCR Burst and Enable INCRx Burst */
+ cfg &= ~DWC3_GSBUSCFG0_INCRBRST_MASK;
+ if (*dwc->incr_burst_type)
+ cfg |= DWC3_GSBUSCFG0_INCRBRSTENA;
+ switch (*(dwc->incr_burst_type + 1)) {
+ case 256:
+ cfg |= DWC3_GSBUSCFG0_INCR256BRSTENA;
+ break;
+ case 128:
+ cfg |= DWC3_GSBUSCFG0_INCR128BRSTENA;
+ break;
+ case 64:
+ cfg |= DWC3_GSBUSCFG0_INCR64BRSTENA;
+ break;
+ case 32:
+ cfg |= DWC3_GSBUSCFG0_INCR32BRSTENA;
+ break;
+ case 16:
+ cfg |= DWC3_GSBUSCFG0_INCR16BRSTENA;
+ break;
+ case 8:
+ cfg |= DWC3_GSBUSCFG0_INCR8BRSTENA;
+ break;
+ case 4:
+ cfg |= DWC3_GSBUSCFG0_INCR4BRSTENA;
+ break;
+ default:
+ dev_err(dev, "Invalid property\n");
+ break;
+ }
+ }
+
+ dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
+}
+
/**
* dwc3_core_init - Low-level initialization of DWC3 Core
* @dwc: Pointer to our controller context structure
@@ -698,6 +747,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
/* Adjust Frame Length */
dwc3_frame_length_adjustment(dwc);
+ dwc3_set_soc_bus_cfg(dwc);
+
usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);
ret = phy_power_on(dwc->usb2_generic_phy);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 065aa6f..cfe389b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -805,6 +805,7 @@ struct dwc3_scratchpad_array {
* @regs: base address for our registers
* @regs_size: address space size
* @fladj: frame length adjustment
+ * @incr_burst_type: INCR burst type adjustment
* @irq_gadget: peripheral controller's IRQ number
* @nr_scratch: number of scratch buffers
* @u1u2: only used on revisions <1.83a for workaround
@@ -928,6 +929,12 @@ struct dwc3 {
enum usb_phy_interface hsphy_mode;
u32 fladj;
+ /*
+* For INCR burst type.
+* First field: for undefined length INCR burst type enable.
+* Second field: for INCRx burst type enable
+*/
+ u32 incr_burst_type[2];
u32 irq_gadget;
u32 nr_scratch;
u32 u1u2;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] USB3/DWC3: Add property "snps,incr-burst-type-adjustment" for INCR burst type
New property "snps,incr-burst-type-adjustment = , " for USB3.0 DWC3.
Field "x": 1/0 - undefined length INCR burst type enable or not;
Field "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst type.
While enabling undefined length INCR burst type and INCR16 burst type,
get better write performance on NXP Layerscape platform:
around 3% improvement (from 364MB/s to 375MB/s).
Signed-off-by: Changming Huang
---
Changes in v3:
- add new property for INCR burst in usb node.
Documentation/devicetree/bindings/usb/dwc3.txt |5 +
arch/arm/boot/dts/ls1021a.dtsi |1 +
arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |3 +++
arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |2 ++
4 files changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index e3e6983..8c405a3 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -55,6 +55,10 @@ Optional properties:
fladj_30mhz_sdbnd signal is invalid or incorrect.
- tx-fifo-resize: determines if the FIFO *has* to be reallocated.
+ - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
+ register, undefined length INCR burst type enable and INCRx type.
+ First field is for undefined length INCR burst type enable or not.
+ Second field is for largest INCRx type enabled.
This is usually a subnode to DWC3 glue to which it is connected.
@@ -63,4 +67,5 @@ dwc3@4a03 {
reg = <0x4a03 0xcfff>;
interrupts = <0 92 4>
usb-phy = <&usb2_phy>, <&usb3,phy>;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 368e219..2999edb 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -627,6 +627,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
pcie@340 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index 97d331e..64828ce 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -482,6 +482,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
usb1: usb3@300 {
@@ -491,6 +492,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
usb2: usb3@310 {
@@ -500,6 +502,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
sata: sata@320 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
index d058e56..414af27 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi
@@ -710,6 +710,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
usb1: usb3@311 {
@@ -720,6 +721,7 @@
dr_mode = "host";
snps,quirk-frame-length-adjustment = <0x20>;
snps,dis_rxdet_inp3_quirk;
+ snps,incr-burst-type-adjustment = <0x1>, <16>;
};
ccn@400 {
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] USB3/DWC3: Add definition for global soc bus configuration register
Add the macro definition for global soc bus configuration register 0/1 Signed-off-by: Changming Huang --- Changes in v3: - no change Changes in v2: - split the patch - add more macro definition for soc bus configuration register drivers/usb/dwc3/core.h | 26 ++ 1 file changed, 26 insertions(+) diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index de5a857..065aa6f 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -161,6 +161,32 @@ /* Bit fields */ +/* Global SoC Bus Configuration Register 0 */ +#define AXI3_CACHE_TYPE_AW 0x8 /* write allocate */ +#define AXI3_CACHE_TYPE_AR 0x4 /* read allocate */ +#define AXI3_CACHE_TYPE_SNP0x2 /* cacheable */ +#define AXI3_CACHE_TYPE_BUF0x1 /* bufferable */ +#define DWC3_GSBUSCFG0_DATARD_SHIFT28 +#define DWC3_GSBUSCFG0_DESCRD_SHIFT24 +#define DWC3_GSBUSCFG0_DATAWR_SHIFT20 +#define DWC3_GSBUSCFG0_DESCWR_SHIFT16 +#define DWC3_GSBUSCFG0_SNP_MASK0x +#define DWC3_GSBUSCFG0_DATABIGEND (1 << 11) +#define DWC3_GSBUSCFG0_DESCBIGEND (1 << 10) +#define DWC3_GSBUSCFG0_INCR256BRSTENA (1 << 7) /* INCR256 burst */ +#define DWC3_GSBUSCFG0_INCR128BRSTENA (1 << 6) /* INCR128 burst */ +#define DWC3_GSBUSCFG0_INCR64BRSTENA (1 << 5) /* INCR64 burst */ +#define DWC3_GSBUSCFG0_INCR32BRSTENA (1 << 4) /* INCR32 burst */ +#define DWC3_GSBUSCFG0_INCR16BRSTENA (1 << 3) /* INCR16 burst */ +#define DWC3_GSBUSCFG0_INCR8BRSTENA(1 << 2) /* INCR8 burst */ +#define DWC3_GSBUSCFG0_INCR4BRSTENA(1 << 1) /* INCR4 burst */ +#define DWC3_GSBUSCFG0_INCRBRSTENA (1 << 0) /* undefined length enable */ +#define DWC3_GSBUSCFG0_INCRBRST_MASK 0xff + +/* Global SoC Bus Configuration Register 1 */ +#define DWC3_GSBUSCFG1_1KPAGEENA (1 << 12) /* 1K page boundary enable */ +#define DWC3_GSBUSCFG1_PTRANSLIMIT_MASK0xf00 + /* Global Debug Queue/FIFO Space Available Register */ #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f) #define DWC3_GDBGFIFOSPACE_TYPE(n) (((n) << 5) & 0x1e0) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 13.12.2016 05:21, Baolin Wang wrote:
Hi Mathias,
On 12 December 2016 at 23:52, Mathias Nyman
wrote:
On 05.12.2016 09:51, Baolin Wang wrote:
If a command event is found on the event ring during an interrupt,
we need to stop the command timer with del_timer(). Since del_timer()
can fail if the timer is running and waiting on the xHCI lock, then
it maybe get the wrong timeout command in xhci_handle_command_timeout()
if host fetched a new command and updated the xhci->current_cmd in
handle_cmd_completion(). For this situation, we need a way to signal
to the command timer that everything is fine and it should exit.
Ah, right, this could actually happen.
We should introduce a counter (xhci->current_cmd_pending) for the number
of pending commands. If we need to cancel the command timer and
del_timer()
succeeds, we decrement the number of pending commands. If del_timer()
fails,
we leave the number of pending commands alone.
For handling timeout command, in xhci_handle_command_timeout() we will
check
the counter after decrementing it, if the counter
(xhci->current_cmd_pending)
is 0, which means xhci->current_cmd is the right timeout command. If the
counter (xhci->current_cmd_pending) is greater than 0, which means current
timeout command has been handled by host and host has fetched new command
as
xhci->current_cmd, then just return and wait for new current command.
A counter like this could work.
Writing the abort bit can generate either ABORT+STOP, or just STOP
event, this seems to cover both.
quick check, case 1: timeout and cmd completion at the same time.
cpu1cpu2
queue_command(first), p++ (=1)
queue_command(more),
--completion irq fires---- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock ) spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
cur_cmd = list_next(), p++ (=2)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=1)
if (p > 0), exit
OK works
case 2: normal timeout case with ABORT+STOP, no race.
cpu1cpu2
queue_command(first), p++ (=1)
queue_command(more),
handle_cmd_timeout()
p-- (P=0), don't exit
mod_timer(), p++ (P=1)
write_abort_bit()
handle_cmd_comletion(ABORT)
del_timer(), ok, p-- (p = 0)
handle_cmd_completion(STOP)
del_timer(), fail, (P=0)
handle_stopped_cmd_ring()
cur_cmd = list_next(), p++ (=1)
mod_timer()
OK, works, and same for just STOP case, with the only difference that
during handle_cmd_completion(STOP) p is decremented (p--)
Yes, that's the cases what I want to handle, thanks for your explicit
explanation.
Gave this some more thought over the weekend, and this implementation
doesn't solve the case when the last command times out and races with the
completion handler:
cpu1cpu2
queue_command(first), p++ (=1)
--completion irq fires---- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock )spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
no more commands, P (=1, nochange)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=0)
p == 0, continue, even if we should not.
For this we still need to rely on checking cur_cmd == NULL in the timeout function.
(Baolus patch sets it to NULL if there are no more commands pending)
And then we could replace the whole counter with a simple check if the timeout
timer
is pending in the timeout function:
xhci_handle_command_timeout()
lock()
if (!cur_cmd || timer_pending(timeout_timer)) {
unlock();
return;
}
-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: xhci: mem: convert to a switch statement
Hi again, Felipe Balbi writes: > Hi, > > Felipe Balbi writes: >> when getting endpoint type a switch statement looks >> better than a series of if () branches. There are no >> functional changes with this patch, cleanup only. >> >> Signed-off-by: Felipe Balbi > > ping here too. Any comments? any comments? -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/2] usb: xhci: switch to running avg trb length
Hi, Felipe Balbi writes: > Hi, > > Felipe Balbi writes: >> It's unlikely that we will ever know the avg so >> instead of assuming it'll be something really large, >> we will calculate the avg as we go as mentioned in >> XHCI specification section 4.14.1.1. >> >> Signed-off-by: Felipe Balbi > > ping?!? This has been here for quite some time. same here, comments? -- balbi signature.asc Description: PGP signature
Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings
On Sat, Dec 17, 2016 at 03:27:43AM -0800, Russell Senior wrote: > All testing is with minicom. Thanks for this through report. > Starting with 00013-gc510871: > > In 8-bit mode, interoperates correctly with pl2303. Switching to > 5-bit mode on both sides (I get unicode boxes of indeterminate > values). Switching back to 8-bit mode on both sides, I get correct > text out both sides. Switching just the ch341 side back to 5-bit > mode, I get 0xff's out on the pl2303 side (still in 8-bit mode). This > would seem to imply that byte-size changes are working on this > version. Changing baud rate also works. Can you verify also what comes through in 5-bit mode is indeed what's expected (e.g. 'a' -> 0x01, 'b' -> 0x02, ...)? You can enable usb-serial debugging to get a log of what is received: modprobe usbserial dyndbg==p > 00014-g79e475a: > > dmesg shows on insert: > > [ 1430.504116] usb 6-2: new full-speed USB device number 3 using uhci_hcd > [ 1430.682143] usb 6-2: New USB device found, idVendor=1a86, idProduct=7523 > [ 1430.682148] usb 6-2: New USB device strings: Mfr=0, Product=2, > SerialNumber=0 > [ 1430.682152] usb 6-2: Product: USB2.0-Ser! > [ 1430.686294] ch341 6-2:1.0: ch341-uart converter detected > [ 1430.689122] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (pre version) > [ 1430.693121] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post init-0) > [ 1430.693124] usb 6-2: ch341_set_baudrate_lcr - speed = 9600, lcr = > c3, a = b202 > [ 1430.696120] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1430.699119] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > [ 1430.700501] usb 6-2: ch341-uart converter now attached to ttyUSB0 > > Changing baud rate and then byte-size: > > [ 1621.189700] usb 6-2: ch341_set_baudrate_lcr - speed = 9600, lcr = > c3, a = b202 > [ 1621.193556] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1621.196557] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > [ 1621.199565] usb 6-2: ch341_set_baudrate_lcr - speed = 115200, lcr = > c3, a = cc03 > [ 1621.202571] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1621.205569] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > [ 1621.207572] usb 6-2: ch341_set_baudrate_lcr - speed = 115200, lcr = > c3, a = cc03 > [ 1621.210570] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1621.213573] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > [ 1644.117396] usb 6-2: ch341_set_baudrate_lcr - speed = 9600, lcr = > c3, a = b202 > [ 1644.120933] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1644.123952] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > [ 1666.950341] usb 6-2: ch341_set_baudrate_lcr - speed = 9600, lcr = > c0, a = b202 > [ 1666.954320] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post divisor) > [ 1666.957316] usb 6-2: ch341_dbg - [0x2518] = c0 00, [0x1213] = 00 00 > (post lcr) > [ 1700.500318] usb 6-2: ch341_set_baudrate_lcr - speed = 9600, lcr = > c3, a = b202 > [ 1700.503845] usb 6-2: ch341_dbg - [0x2518] = c0 00, [0x1213] = 00 00 > (post divisor) > [ 1700.506866] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (post lcr) > > It works in 8-bit mode, I can change to 5-bit mode and then back, as > before. And I can change baud rate. Note that you only need to test this commit, which simply adds the debugging printks on top of the previous. The log indicates that the LCR is updated correctly, but also reveals that the divisor registers always read back as zero even if changing speeds appear to work. Both my devices read back the values set either through direct register write or using the init commmand. We may be able to use this to detect these devices and fall back to direct register writes. > 00015-gee5a27b: > I can switch baud rate and byte size back and forth and it seems to > work. In 5-bit mode, I guess minicom just doesn't know how to render > the values, so that makes sense, switching back to 8-bit mode and I > get readable text again. So, this one is working too, afaict. Good, this one only differs from the previous in that the 7th bit of the divisor register is set, something which is required for CH341A. Good to know it does not seem to have any negative effects for this device. > 00016-ga06b45d: > > dmesg on insert: > > [ 2588.016052] usb 6-2: new full-speed USB device number 5 using uhci_hcd > [ 2588.189182] usb 6-2: New USB device found, idVendor=1a86, idProduct=7523 > [ 2588.189187] usb 6-2: New USB device strings: Mfr=0, Product=2, > SerialNumber=0 > [ 2588.189192] usb 6-2: Product: USB2.0-Ser! > [ 2588.193366] ch341 6-2:1.0: ch341-uart converter detected > [ 2588.196146] usb 6-2: ch341_dbg - [0x2518] = c3 00, [0x1213] = 00 00 > (pre version) > [ 2588.200162] usb 6-2: ch341_dbg -
Re: [PATCH] USB: cypress_m8: remove unused variable
On Sun, Dec 18, 2016 at 10:29:27PM +, Sudip Mukherjee wrote: > The variable havedata was only being set but never used afterwards. > > Signed-off-by: Sudip Mukherjee Thanks, I'll queue this up for 4.11. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: serial: f81534: Detect errors from f81534_logic_to_phy_port()
On Sun, Dec 18, 2016 at 10:19:28AM +0100, Geert Uytterhoeven wrote:
> With gcc 4.1.2:
>
> drivers/usb/serial/f81534.c: In function ‘f81534_port_probe’:
> drivers/usb/serial/f81534.c:1250: warning: comparison is always false due
> to limited range of data type
>
> f81534_logic_to_phy_port() may return a negative error value, which is
> ignored by assigning it to u8 f81534_port_private.phy_num.
>
> Use an intermediate variable of type int to fix this.
> While at it, forward the actual error code instead of converting it to
> -ENODEV, and drop the useless check for F81534_NUM_PORT, as the callee
> always returns a valid port number in case of success.
>
> Fixes: 0c9bd6004d258d46 ("USB: serial: add Fintek F81532/534 driver")
> Signed-off-by: Geert Uytterhoeven
> ---
> v2:
> - Drop useless check for F81534_NUM_PORT.
Thanks for the update. I'll queue this one up for -rc2.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi Mathias, On 19 December 2016 at 18:33, Mathias Nyman wrote: > On 13.12.2016 05:21, Baolin Wang wrote: >> >> Hi Mathias, >> >> On 12 December 2016 at 23:52, Mathias Nyman >> wrote: >>> >>> On 05.12.2016 09:51, Baolin Wang wrote: If a command event is found on the event ring during an interrupt, we need to stop the command timer with del_timer(). Since del_timer() can fail if the timer is running and waiting on the xHCI lock, then it maybe get the wrong timeout command in xhci_handle_command_timeout() if host fetched a new command and updated the xhci->current_cmd in handle_cmd_completion(). For this situation, we need a way to signal to the command timer that everything is fine and it should exit. >>> >>> >>> >>> Ah, right, this could actually happen. >>> We should introduce a counter (xhci->current_cmd_pending) for the number of pending commands. If we need to cancel the command timer and del_timer() succeeds, we decrement the number of pending commands. If del_timer() fails, we leave the number of pending commands alone. For handling timeout command, in xhci_handle_command_timeout() we will check the counter after decrementing it, if the counter (xhci->current_cmd_pending) is 0, which means xhci->current_cmd is the right timeout command. If the counter (xhci->current_cmd_pending) is greater than 0, which means current timeout command has been handled by host and host has fetched new command as xhci->current_cmd, then just return and wait for new current command. >>> >>> >>> >>> A counter like this could work. >>> >>> Writing the abort bit can generate either ABORT+STOP, or just STOP >>> event, this seems to cover both. >>> >>> quick check, case 1: timeout and cmd completion at the same time. >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> queue_command(more), >>> --completion irq fires---- timer times out at same time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock ) spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> cur_cmd = list_next(), p++ (=2) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=1) >>> if (p > 0), exit >>> OK works >>> >>> case 2: normal timeout case with ABORT+STOP, no race. >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> queue_command(more), >>> handle_cmd_timeout() >>> p-- (P=0), don't exit >>> mod_timer(), p++ (P=1) >>> write_abort_bit() >>> handle_cmd_comletion(ABORT) >>> del_timer(), ok, p-- (p = 0) >>> handle_cmd_completion(STOP) >>> del_timer(), fail, (P=0) >>> handle_stopped_cmd_ring() >>> cur_cmd = list_next(), p++ (=1) >>> mod_timer() >>> >>> OK, works, and same for just STOP case, with the only difference that >>> during handle_cmd_completion(STOP) p is decremented (p--) >> >> >> Yes, that's the cases what I want to handle, thanks for your explicit >> explanation. >> > > Gave this some more thought over the weekend, and this implementation > doesn't solve the case when the last command times out and races with the > completion handler: > > cpu1cpu2 > > queue_command(first), p++ (=1) > --completion irq fires---- timer times out at same time-- > handle_cmd_completion() handle_cmd_timeout(),) > lock(xhci_lock )spin_on(xhci_lock) > del_timer() fail, p (=1, nochange) > no more commands, P (=1, nochange) > unlock(xhci_lock) > lock(xhci_lock) > p-- (=0) > p == 0, continue, even if we should > not. > For this we still need to rely on > checking cur_cmd == NULL in the timeout function. > (Baolus patch sets it to NULL if there are no more commands pending) As I pointed out in patch 1 of this patchset, this patchset is based on Lu Baolu's new fix patch: usb: xhci: fix possible wild pointer https://www.spinics.net/lists/linux-usb/msg150219.html After applying Baolu's patch, after decrement the counter, we will check the xhci->cur_command if is NULL. So in this situation: cpu1cpu2 queue_command(first), p++ (=1) --completion irq fires---- timer times out at same time-- handle_cmd_completion() handle_cmd_timeout(),) lock(xhci_lock )spin_on(xhci_lock) del_timer() fail, p (=1, nochange) no more commands, P (=1, nochange) unlock(
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 19.12.2016 13:34, Baolin Wang wrote:
Hi Mathias,
On 19 December 2016 at 18:33, Mathias Nyman
wrote:
On 13.12.2016 05:21, Baolin Wang wrote:
Hi Mathias,
On 12 December 2016 at 23:52, Mathias Nyman
wrote:
On 05.12.2016 09:51, Baolin Wang wrote:
If a command event is found on the event ring during an interrupt,
we need to stop the command timer with del_timer(). Since del_timer()
can fail if the timer is running and waiting on the xHCI lock, then
it maybe get the wrong timeout command in xhci_handle_command_timeout()
if host fetched a new command and updated the xhci->current_cmd in
handle_cmd_completion(). For this situation, we need a way to signal
to the command timer that everything is fine and it should exit.
Ah, right, this could actually happen.
We should introduce a counter (xhci->current_cmd_pending) for the number
of pending commands. If we need to cancel the command timer and
del_timer()
succeeds, we decrement the number of pending commands. If del_timer()
fails,
we leave the number of pending commands alone.
For handling timeout command, in xhci_handle_command_timeout() we will
check
the counter after decrementing it, if the counter
(xhci->current_cmd_pending)
is 0, which means xhci->current_cmd is the right timeout command. If the
counter (xhci->current_cmd_pending) is greater than 0, which means
current
timeout command has been handled by host and host has fetched new
command
as
xhci->current_cmd, then just return and wait for new current command.
A counter like this could work.
Writing the abort bit can generate either ABORT+STOP, or just STOP
event, this seems to cover both.
quick check, case 1: timeout and cmd completion at the same time.
cpu1cpu2
queue_command(first), p++ (=1)
queue_command(more),
--completion irq fires---- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock ) spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
cur_cmd = list_next(), p++ (=2)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=1)
if (p > 0), exit
OK works
case 2: normal timeout case with ABORT+STOP, no race.
cpu1cpu2
queue_command(first), p++ (=1)
queue_command(more),
handle_cmd_timeout()
p-- (P=0), don't exit
mod_timer(), p++ (P=1)
write_abort_bit()
handle_cmd_comletion(ABORT)
del_timer(), ok, p-- (p = 0)
handle_cmd_completion(STOP)
del_timer(), fail, (P=0)
handle_stopped_cmd_ring()
cur_cmd = list_next(), p++ (=1)
mod_timer()
OK, works, and same for just STOP case, with the only difference that
during handle_cmd_completion(STOP) p is decremented (p--)
Yes, that's the cases what I want to handle, thanks for your explicit
explanation.
Gave this some more thought over the weekend, and this implementation
doesn't solve the case when the last command times out and races with the
completion handler:
cpu1cpu2
queue_command(first), p++ (=1)
--completion irq fires---- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock )spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
no more commands, P (=1, nochange)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=0)
p == 0, continue, even if we should
not.
For this we still need to rely on
checking cur_cmd == NULL in the timeout function.
(Baolus patch sets it to NULL if there are no more commands pending)
As I pointed out in patch 1 of this patchset, this patchset is based
on Lu Baolu's new fix patch:
usb: xhci: fix possible wild pointer
https://www.spinics.net/lists/linux-usb/msg150219.html
After applying Baolu's patch, after decrement the counter, we will
check the xhci->cur_command if is NULL. So in this situation:
cpu1cpu2
queue_command(first), p++ (=1)
--completion irq fires---- timer times out at same time--
handle_cmd_completion() handle_cmd_timeout(),)
lock(xhci_lock )spin_on(xhci_lock)
del_timer() fail, p (=1, nochange)
no more commands, P (=1, nochange)
unlock(xhci_lock)
lock(xhci_lock)
p-- (=0)
no current command, return
if (!xhci->current_cmd) {
unlock(xhci_lock);
[RFC PATCH] usb: USB Type-C connector class
The purpose of USB Type-C connector class is to provide unified interface for the user space to get the status and basic information about USB Type-C connectors on a system, control over data role swapping, and when the port supports USB Power Delivery, also control over power role swapping and Alternate Modes. Signed-off-by: Heikki Krogerus --- Documentation/ABI/testing/sysfs-class-typec | 220 ++ Documentation/usb/typec.txt | 174 + MAINTAINERS |9 + drivers/usb/Kconfig |2 + drivers/usb/Makefile|2 + drivers/usb/typec/Kconfig |7 + drivers/usb/typec/Makefile |1 + drivers/usb/typec/typec.c | 1047 +++ include/linux/usb/typec.h | 212 ++ 9 files changed, 1674 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-typec create mode 100644 Documentation/usb/typec.txt create mode 100644 drivers/usb/typec/Kconfig create mode 100644 drivers/usb/typec/Makefile create mode 100644 drivers/usb/typec/typec.c create mode 100644 include/linux/usb/typec.h Hi, There was one more change to the ABI: the ports are now named "port" instead of "usbc". Let me know if that is OK! The series is still being reviewed internally, but I'm sending this already here so you guys can take a look. The API now expects the drivers to register every part separately, including the alternate modes for the port itself. Structures for the partner/cable/plug/altmode are now protected, and the drivers need to register those with separate descriptor structures and get a handle to struct typec_partner/cable/plug/altmode if the registration is successful, or just NULL in case of failure as Greg proposed. As the drivers need to register the partners and the cables separately in any case, the drivers do not declare connections with the class anymore. If the default role changes during initial negotiation with a partner, the drivers need to notify the class before registering the partner or cable. Thanks, diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec new file mode 100644 index 000..663709a --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-typec @@ -0,0 +1,220 @@ +USB Type-C port devices (eg. /sys/class/typec/port0/) + +What: /sys/class/typec//current_data_role +Date: February 2017 +Contact: Heikki Krogerus +Description: + The current USB data role the port is operating in. This + attribute can be used for requesting data role swapping on the + port. Swapping is supported as synchronous operation, so + write(2) to the attribute will not return until the operation + has finished. The attribute is notified about role changes so + that poll(2) on the attribute wakes up. Change on the role will + also generate uevent KOBJ_CHANGE on the port. + + Valid values: + - host + - device + +What: /sys/class/typec//current_power_role +Date: February 2017 +Contact: Heikki Krogerus +Description: + The current power role of the port. This attribute can be used + to request power role swap on the port when the port supports + USB Power Delivery. Swapping is supported as synchronous + operation, so write(2) to the attribute will not return until + the operation has finished. The attribute is notified about role + changes so that poll(2) on the attribute wakes up. Change on the + role will also generate uevent KOBJ_CHANGE. + + Valid values: + - source + - sink + +What: /sys/class/typec//vconn_source +Date: February 2017 +Contact: Heikki Krogerus +Description: + Shows is the port VCONN Source. This attribute can be used to + request VCONN swap to change the VCONN Source during connection + when both the port and the partner support USB Power Delivery. + Swapping is supported as synchronous operation, so write(2) to + the attribute will not return until the operation has finished. + The attribute is notified about VCONN source changes so that + poll(2) on the attribute wakes up. Change on VCONN source also + generates uevent KOBJ_CHANGE. + + Valid values are: + - 0 when the port is not the VCONN Source + - 1 when the port is the VCONN Source + +What: /sys/class/typec//power_operation_mode +Date: February 2017 +Contact: Heikki Krogerus +Description: + Shows the current power operational mode the port is in. + +
Re: [PATCH v2 2/4] usb: dwc2: Add binding for AHB burst
Hello John, hello Felipe
On Monday, November 28, 2016 7:32:20 PM CET John Youn wrote:
> On 11/22/2016 12:51 PM, Christian Lamparter wrote:
> > On Monday, November 21, 2016 7:32:30 PM CET John Youn wrote:
> >> On 11/21/2016 1:10 PM, Christian Lamparter wrote:
> >>> On Monday, November 21, 2016 12:16:31 PM CET John Youn wrote:
> On 11/18/2016 12:18 PM, Christian Lamparter wrote:
> > On Friday, November 18, 2016 8:16:08 AM CET Rob Herring wrote:
> >> Also, perhaps you should allow that the compatible string can define
> >> the
> >> default.
> >>
> > I hoped you would say that :).
> >
> > I've attached a patch (on top of John Youn changes) [...]
> > ---
> > Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for
> > amcc,dwc-otg
> > [...]
> > @@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
> > +/* [...] */
> > +static const struct of_device_id dwc2_compat_ahb_bursts[] = {
> > + {
> > + .compatible = "amcc,dwc-otg",
> > + .data = (void *) GAHBCFG_HBSTLEN_INCR16,
> > + },
> > +};
> >> [...]
> >> @@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct
> >> dwc2_hsotg *hsotg)
> > ret = device_property_read_string(hsotg->dev, "snps,ahb-burst",
> > &str);
> > if (ret < 0) {
> > + const struct of_device_id *match;
> > +
> > + match = of_match_node(dwc2_compat_ahb_bursts, node);
> > + if (match)
> > + ret = (int)match->data;
> > +
> >> [...]
> I'd prefer if you use the binding which requires no extra code in
> dwc2.
> >>> I'm fine with either option. However it think that this would require
> >>> that either Mark or Rob would allow an exception to the "keep existing
> >>> dts the way they are) and ack the following change to the
> >>> canyonlands.dts.
> >> [...]
>
> Ok thanks for clearing that up. I understand.
>
> For now we can just set the property to "INCR16" based on the
> compatible string. Perhaps in the future do this from a glue-layer
> driver which binds to all compatible strings other than "snps,dwc2".
>
> I won't be able to do anything with this until next week though.
Ok, I think enough time has passed. I would like to see this
patch series (v3 [0]) being queued for 4.11+ together with
"usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg" [1].
Felipe, if you want I can resend the series and add the
"amcc,dwc-otg" patch to it as well. Just let me know what you
prefer here.
Regards,
Christian
[0]
[1]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: musb: debugfs: allow forcing host mode together with speed in testmode
On Sun, Dec 18, 2016 at 12:54:40AM +0100, Pali Rohár wrote: > Based on the musb ug, force_host bit is allowed to be set along with > force_hs or force_fs bit. > > It could help to implement forced host mode via testmode on Nokia N900. > > Signed-off-by: Pali Rohár > --- > Changes in v2: > * Use == instead of & for comparison of testmode > --- > drivers/usb/musb/musb_debugfs.c | 44 > +-- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c > index 9b22d94..5308cea 100644 > --- a/drivers/usb/musb/musb_debugfs.c > +++ b/drivers/usb/musb/musb_debugfs.c > @@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void > *unused) > > test = musb_readb(musb->mregs, MUSB_TESTMODE); > > - if (test & MUSB_TEST_FORCE_HOST) > + if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS)) > + seq_printf(s, "force host full-speed\n"); > + > + else if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS)) > + seq_printf(s, "force host high-speed\n"); > + > + else if (test == MUSB_TEST_FORCE_HOST) > seq_printf(s, "force host\n"); > > - if (test & MUSB_TEST_FIFO_ACCESS) > + else if (test == MUSB_TEST_FIFO_ACCESS) > seq_printf(s, "fifo access\n"); > > - if (test & MUSB_TEST_FORCE_FS) > + else if (test == MUSB_TEST_FORCE_FS) > seq_printf(s, "force full-speed\n"); > > - if (test & MUSB_TEST_FORCE_HS) > + else if (test == MUSB_TEST_FORCE_HS) > seq_printf(s, "force high-speed\n"); > > - if (test & MUSB_TEST_PACKET) > + else if (test == MUSB_TEST_PACKET) > seq_printf(s, "test packet\n"); > > - if (test & MUSB_TEST_K) > + else if (test == MUSB_TEST_K) > seq_printf(s, "test K\n"); > > - if (test & MUSB_TEST_J) > + else if (test == MUSB_TEST_J) > seq_printf(s, "test J\n"); > > - if (test & MUSB_TEST_SE0_NAK) > + else if (test == MUSB_TEST_SE0_NAK) > seq_printf(s, "test SE0 NAK\n"); > > return 0; > @@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file, > if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count))) > return -EFAULT; > > - if (strstarts(buf, "force host")) > + if (strstarts(buf, "force host full-speed")) buf is only 18 bytes long, the string is not fully compared. You might have to increase buf[] to 24 bytes. Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] usb: musb: debugfs: allow forcing host mode together with speed in testmode
Based on the musb ug, force_host bit is allowed to be set along with
force_hs or force_fs bit.
It could help to implement forced host mode via testmode on Nokia N900.
Signed-off-by: Pali Rohár
---
Changes in v2:
* Use == instead of & for comparison of testmode
Changes in v3:
* Increase buf[] to 24 bytes
---
drivers/usb/musb/musb_debugfs.c | 46 ---
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
index 9b22d94..4d6c505 100644
--- a/drivers/usb/musb/musb_debugfs.c
+++ b/drivers/usb/musb/musb_debugfs.c
@@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void
*unused)
test = musb_readb(musb->mregs, MUSB_TESTMODE);
- if (test & MUSB_TEST_FORCE_HOST)
+ if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
+ seq_printf(s, "force host full-speed\n");
+
+ else if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
+ seq_printf(s, "force host high-speed\n");
+
+ else if (test == MUSB_TEST_FORCE_HOST)
seq_printf(s, "force host\n");
- if (test & MUSB_TEST_FIFO_ACCESS)
+ else if (test == MUSB_TEST_FIFO_ACCESS)
seq_printf(s, "fifo access\n");
- if (test & MUSB_TEST_FORCE_FS)
+ else if (test == MUSB_TEST_FORCE_FS)
seq_printf(s, "force full-speed\n");
- if (test & MUSB_TEST_FORCE_HS)
+ else if (test == MUSB_TEST_FORCE_HS)
seq_printf(s, "force high-speed\n");
- if (test & MUSB_TEST_PACKET)
+ else if (test == MUSB_TEST_PACKET)
seq_printf(s, "test packet\n");
- if (test & MUSB_TEST_K)
+ else if (test == MUSB_TEST_K)
seq_printf(s, "test K\n");
- if (test & MUSB_TEST_J)
+ else if (test == MUSB_TEST_J)
seq_printf(s, "test J\n");
- if (test & MUSB_TEST_SE0_NAK)
+ else if (test == MUSB_TEST_SE0_NAK)
seq_printf(s, "test SE0 NAK\n");
return 0;
@@ -192,7 +198,7 @@ static ssize_t musb_test_mode_write(struct file *file,
struct seq_file *s = file->private_data;
struct musb *musb = s->private;
u8 test;
- charbuf[18];
+ charbuf[24];
test = musb_readb(musb->mregs, MUSB_TESTMODE);
if (test) {
@@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file,
if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
return -EFAULT;
- if (strstarts(buf, "force host"))
+ if (strstarts(buf, "force host full-speed"))
+ test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
+
+ else if (strstarts(buf, "force host high-speed"))
+ test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
+
+ else if (strstarts(buf, "force host"))
test = MUSB_TEST_FORCE_HOST;
- if (strstarts(buf, "fifo access"))
+ else if (strstarts(buf, "fifo access"))
test = MUSB_TEST_FIFO_ACCESS;
- if (strstarts(buf, "force full-speed"))
+ else if (strstarts(buf, "force full-speed"))
test = MUSB_TEST_FORCE_FS;
- if (strstarts(buf, "force high-speed"))
+ else if (strstarts(buf, "force high-speed"))
test = MUSB_TEST_FORCE_HS;
- if (strstarts(buf, "test packet")) {
+ else if (strstarts(buf, "test packet")) {
test = MUSB_TEST_PACKET;
musb_load_testpacket(musb);
}
- if (strstarts(buf, "test K"))
+ else if (strstarts(buf, "test K"))
test = MUSB_TEST_K;
- if (strstarts(buf, "test J"))
+ else if (strstarts(buf, "test J"))
test = MUSB_TEST_J;
- if (strstarts(buf, "test SE0 NAK"))
+ else if (strstarts(buf, "test SE0 NAK"))
test = MUSB_TEST_SE0_NAK;
musb_writeb(musb->mregs, MUSB_TESTMODE, test);
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc2: gadget: Fix fifo size configuration
Currently the upper limit for the endpoint index during fifo size
config was always 16 instead of the available endpoints. So fix this
by using the determined amount of endpoints and avoid a warning about
"insufficient fifo memory" on bcm2835 which has only 8 endpoints.
Signed-off-by: Stefan Wahren
Fixes: 0a176279db68 ("usb: dwc2: gadget: configure fifos from device tree")
---
drivers/usb/dwc2/gadget.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b95930f..b00184c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -228,7 +228,7 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
* them to endpoints dynamically according to maxpacket size value of
* given endpoint.
*/
- for (ep = 1; ep < MAX_EPS_CHANNELS; ep++) {
+ for (ep = 1; ep < hsotg->num_of_eps; ep++) {
if (!txfsz[ep])
continue;
val = addr;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFT] usb: dwc2: gadget: Fix fifo size configuration
Currently the upper limit for the endpoint index during fifo size
config was always 16 instead of the available endpoints. So fix this
by using the determined amount of endpoints and avoid a warning about
"insufficient fifo memory" on bcm2835 which has only 8 endpoints.
Signed-off-by: Stefan Wahren
Fixes: 0a176279db68 ("usb: dwc2: gadget: configure fifos from device tree")
---
drivers/usb/dwc2/gadget.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Since revert of ("usb: dwc2: gadget: fix TX FIFO size and address") which
caused regressions on some platforms this is the second attempt to fix
gadget support for bcm2835.
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b95930f..b00184c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -228,7 +228,7 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
* them to endpoints dynamically according to maxpacket size value of
* given endpoint.
*/
- for (ep = 1; ep < MAX_EPS_CHANNELS; ep++) {
+ for (ep = 1; ep < hsotg->num_of_eps; ep++) {
if (!txfsz[ep])
continue;
val = addr;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings
On Mon, Dec 19, 2016 at 2:58 AM, Johan Hovold wrote: > On Sat, Dec 17, 2016 at 03:27:43AM -0800, Russell Senior wrote: >> All testing is with minicom. > > Thanks for this through report. > >> Starting with 00013-gc510871: >> >> In 8-bit mode, interoperates correctly with pl2303. Switching to >> 5-bit mode on both sides (I get unicode boxes of indeterminate >> values). Switching back to 8-bit mode on both sides, I get correct >> text out both sides. Switching just the ch341 side back to 5-bit >> mode, I get 0xff's out on the pl2303 side (still in 8-bit mode). This >> would seem to imply that byte-size changes are working on this >> version. Changing baud rate also works. > > Can you verify also what comes through in 5-bit mode is indeed what's > expected (e.g. 'a' -> 0x01, 'b' -> 0x02, ...)? > > You can enable usb-serial debugging to get a log of what is received: > > modprobe usbserial dyndbg==p > Yes, it does seem that I'm getting the 5-bit truncation. Here is an excerpt from the prodigious logs, in this case sending a 'Z' from pl2303 and receiving an 0x5a on the ch341. The same sort of thing, e.g. 'A' (0x41) from the ch341 lands as an 0x01 on the pl2303, 0x42 -> 0x02, etc. Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_write_room Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_tiocmget Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_ioctl - cmd 0x5401 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_write_room Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_write_room Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_tiocmget Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_ioctl - cmd 0x5401 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_write_room Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_write_room Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_write - 1 byte(s) Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_write_start - length = 1, data = 5a Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_tiocmget Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_ioctl - cmd 0x5401 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_chars_in_buffer - returns 1 Dec 19 08:22:53 willard kernel: tty ttyUSB1: serial_write_room Dec 19 08:22:53 willard kernel: pl2303 ttyUSB1: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_read_bulk_callback - urb 1, len 1 Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_read_bulk_callback - length = 1, data = 1a Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_submit_read_urb - urb 1 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_write_room Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_write_room - returns 4096 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_tiocmget Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_ioctl - cmd 0x5401 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_chars_in_buffer Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_chars_in_buffer - returns 0 Dec 19 08:22:53 willard kernel: tty ttyUSB0: serial_write_room Dec 19 08:22:53 willard kernel: ch341-uart ttyUSB0: usb_serial_generic_write_room - returns 4096 >> 00014-g79e475a: >> >> dmesg shows on insert: >> >> [ 1430.504116] usb 6-2: new full-speed USB device number 3 using uhci_hcd >> [ 1430.682143] usb 6-2: New USB device found, idVen
[PATCH] USB: fix problems with duplicate endpoint addresses
When checking a new device's descriptors, the USB core does not check
for duplicate endpoint addresses. This can cause a problem when the
sysfs files for those endpoints are created; trying to create multiple
files with the same name will provoke a WARNING:
WARNING: CPU: 2 PID: 865 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x8a/0xa0
sysfs: cannot create duplicate filename
'/devices/platform/dummy_hcd.0/usb2/2-1/2-1:64.0/ep_05'
Kernel panic - not syncing: panic_on_warn set ...
CPU: 2 PID: 865 Comm: kworker/2:1 Not tainted 4.9.0-rc7+ #34
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
88006bee64c8 81f96b8a 0001 11000d7dcc2c
ed000d7dcc24 0001 41b58ab3 8598b510
81f968f8 850fee20 85cff020 dc00
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[] dump_stack+0x292/0x398 lib/dump_stack.c:51
[] panic+0x1cb/0x3a9 kernel/panic.c:179
[] __warn+0x1c4/0x1e0 kernel/panic.c:542
[] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565
[] sysfs_warn_dup+0x8a/0xa0 fs/sysfs/dir.c:30
[] sysfs_create_dir_ns+0x178/0x1d0 fs/sysfs/dir.c:59
[< inline >] create_dir lib/kobject.c:71
[] kobject_add_internal+0x227/0xa60 lib/kobject.c:229
[< inline >] kobject_add_varg lib/kobject.c:366
[] kobject_add+0x139/0x220 lib/kobject.c:411
[] device_add+0x353/0x1660 drivers/base/core.c:1088
[] device_register+0x1d/0x20 drivers/base/core.c:1206
[] usb_create_ep_devs+0x163/0x260
drivers/usb/core/endpoint.c:195
[] create_intf_ep_devs+0x13b/0x200
drivers/usb/core/message.c:1030
[] usb_set_configuration+0x1083/0x18d0
drivers/usb/core/message.c:1937
[] generic_probe+0x6e/0xe0 drivers/usb/core/generic.c:172
[] usb_probe_device+0xaa/0xe0 drivers/usb/core/driver.c:263
This patch prevents the problem by checking for duplicate endpoint
addresses during enumeration and skipping any duplicates.
Signed-off-by: Alan Stern
Reported-by: Andrey Konovalov
Tested-by: Andrey Konovalov
CC:
---
[as1821]
drivers/usb/core/config.c | 10 ++
1 file changed, 10 insertions(+)
Index: usb-4.x/drivers/usb/core/config.c
===
--- usb-4.x.orig/drivers/usb/core/config.c
+++ usb-4.x/drivers/usb/core/config.c
@@ -234,6 +234,16 @@ static int usb_parse_endpoint(struct dev
if (ifp->desc.bNumEndpoints >= num_ep)
goto skip_to_next_endpoint_or_interface_descriptor;
+ /* Check for duplicate endpoint addresses */
+ for (i = 0; i < ifp->desc.bNumEndpoints; ++i) {
+ if (ifp->endpoint[i].desc.bEndpointAddress ==
+ d->bEndpointAddress) {
+ dev_warn(ddev, "config %d interface %d altsetting %d
has a duplicate endpoint with address 0x%X, skipping\n",
+ cfgno, inum, asnum, d->bEndpointAddress);
+ goto skip_to_next_endpoint_or_interface_descriptor;
+ }
+ }
+
endpoint = &ifp->endpoint[ifp->desc.bNumEndpoints];
++ifp->desc.bNumEndpoints;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] usb: USB Type-C connector class
On 12/19/2016 06:45 AM, Heikki Krogerus wrote: The purpose of USB Type-C connector class is to provide unified interface for the user space to get the status and basic information about USB Type-C connectors on a system, control over data role swapping, and when the port supports USB Power Delivery, also control over power role swapping and Alternate Modes. Signed-off-by: Heikki Krogerus --- Documentation/ABI/testing/sysfs-class-typec | 220 ++ Documentation/usb/typec.txt | 174 + MAINTAINERS |9 + drivers/usb/Kconfig |2 + drivers/usb/Makefile|2 + drivers/usb/typec/Kconfig |7 + drivers/usb/typec/Makefile |1 + drivers/usb/typec/typec.c | 1047 +++ include/linux/usb/typec.h | 212 ++ 9 files changed, 1674 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-typec create mode 100644 Documentation/usb/typec.txt create mode 100644 drivers/usb/typec/Kconfig create mode 100644 drivers/usb/typec/Makefile create mode 100644 drivers/usb/typec/typec.c create mode 100644 include/linux/usb/typec.h Hi, There was one more change to the ABI: the ports are now named "port" instead of "usbc". Let me know if that is OK! The series is still being reviewed internally, but I'm sending this already here so you guys can take a look. The API now expects the drivers to register every part separately, including the alternate modes for the port itself. Structures for the partner/cable/plug/altmode are now protected, and the drivers need to register those with separate descriptor structures and get a handle to struct typec_partner/cable/plug/altmode if the registration is successful, or just NULL in case of failure as Greg proposed. As the drivers need to register the partners and the cables separately in any case, the drivers do not declare connections with the class anymore. If the default role changes during initial negotiation with a partner, the drivers need to notify the class before registering the partner or cable. Can you also publish the Whiskey Cove driver ? That might help figuring out the necessary driver changes. Thanks, Guenter Thanks, diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec new file mode 100644 index 000..663709a --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-typec @@ -0,0 +1,220 @@ +USB Type-C port devices (eg. /sys/class/typec/port0/) + +What: /sys/class/typec//current_data_role +Date: February 2017 +Contact: Heikki Krogerus +Description: + The current USB data role the port is operating in. This + attribute can be used for requesting data role swapping on the + port. Swapping is supported as synchronous operation, so + write(2) to the attribute will not return until the operation + has finished. The attribute is notified about role changes so + that poll(2) on the attribute wakes up. Change on the role will + also generate uevent KOBJ_CHANGE on the port. + + Valid values: + - host + - device + +What: /sys/class/typec//current_power_role +Date: February 2017 +Contact: Heikki Krogerus +Description: + The current power role of the port. This attribute can be used + to request power role swap on the port when the port supports + USB Power Delivery. Swapping is supported as synchronous + operation, so write(2) to the attribute will not return until + the operation has finished. The attribute is notified about role + changes so that poll(2) on the attribute wakes up. Change on the + role will also generate uevent KOBJ_CHANGE. + + Valid values: + - source + - sink + +What: /sys/class/typec//vconn_source +Date: February 2017 +Contact: Heikki Krogerus +Description: + Shows is the port VCONN Source. This attribute can be used to + request VCONN swap to change the VCONN Source during connection + when both the port and the partner support USB Power Delivery. + Swapping is supported as synchronous operation, so write(2) to + the attribute will not return until the operation has finished. + The attribute is notified about VCONN source changes so that + poll(2) on the attribute wakes up. Change on VCONN source also + generates uevent KOBJ_CHANGE. + + Valid values are: + - 0 when the port is not the VCONN Source + - 1 when the port is the VCONN Source + +What: /sys/class/type
Re: [PATCH v10 0/8] power: add power sequence library
On Mon, Nov 14, 2016 at 09:35:51AM +0800, Peter Chen wrote: > Hi all, > > This is a follow-up for my last power sequence framework patch set [1]. > According to Rob Herring and Ulf Hansson's comments[2]. The kinds of > power sequence instances will be added at postcore_initcall, the match > criteria is compatible string first, if the compatible string is not > matched between dts and library, it will try to use generic power sequence. > > The host driver just needs to call of_pwrseq_on/of_pwrseq_off > if only one power sequence instance is needed, for more power sequences > are used, using of_pwrseq_on_list/of_pwrseq_off_list instead (eg, USB hub > driver). > > In future, if there are special power sequence requirements, the special > power sequence library can be created. > > This patch set is tested on i.mx6 sabresx evk using a dts change, I use > two hot-plug devices to simulate this use case, the related binding > change is updated at patch [1/6], The udoo board changes were tested > using my last power sequence patch set.[3] > > Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also > need to power on itself before it can be found by ULPI bus. > > [1] http://www.spinics.net/lists/linux-usb/msg142755.html > [2] http://www.spinics.net/lists/linux-usb/msg143106.html > [3] http://www.spinics.net/lists/linux-usb/msg142815.html > > Changes for v10: > - Improve the kernel-doc for power sequence core, including exported APIs and > main structure. [Patch 2/8] > - Change Kconfig, and let the user choose power sequence. [Patch 2/8] > - Delete EXPORT_SYMBOL and change related APIs as local, these APIs do not > be intended to export currently. [Patch 2/8] > - Selete POWER_SEQUENCE at USB core's Kconfig. [Patch 4/8] Hi Peter, It is great that you continued the work on this. I saw (in some previous mails) your repo mentioned: https://git.kernel.org/cgit/linux/kernel/git/peter.chen/usb.git/ Does it contain the recent version of this patchset? I want to build on top of it fixes for usb3503 on Odroid U3 board. Best regards, Krzysztof > > Changes for v9: > - Add Vaibhav Hiremath's reviewed-by [Patch 4/8] > - Rebase to v4.9-rc1 > > Changes for v8: > - Allocate one extra pwrseq instance if pwrseq_get has succeed, it can avoid > preallocate instances problem which the number of instance is decided at > compile time, thanks for Heiko Stuebner's suggestion [Patch 2/8] > - Delete pwrseq_compatible_sample.c which is the demo purpose to show > compatible > match method. [Patch 2/8] > - Add Maciej S. Szmigiero's tested-by. [Patch 7/8] > > Changes for v7: > - Create kinds of power sequence instance at postcore_initcall, and match > the instance with node using compatible string, the beneit of this is > the host driver doesn't need to consider which pwrseq instance needs > to be used, and pwrseq core will match it, however, it eats some memories > if less power sequence instances are used. [Patch 2/8] > - Add pwrseq_compatible_sample.c to test match pwrseq using device_id. [Patch > 2/8] > - Fix the comments Vaibhav Hiremath adds for error path for clock and do not > use device_node for parameters at pwrseq_on. [Patch 2/8] > - Simplify the caller to use power sequence, follows Alan's commnets [Patch > 4/8] > - Tested three pwrseq instances together using both specific compatible > string and > generic libraries. > > Changes for v6: > - Add Matthias Kaehlcke's Reviewed-by and Tested-by. (patch [2/6]) > - Change chipidea core of_node assignment for coming user. (patch [5/6]) > - Applies Joshua Clayton's three dts changes for two boards, > the USB device's reg has only #address-cells, but without #size-cells. > > Changes for v5: > - Delete pwrseq_register/pwrseq_unregister, which is useless currently > - Fix the linker error when the pwrseq user is compiled as module > > Changes for v4: > - Create the patch on next-20160722 > - Fix the of_node is not NULL after chipidea driver is unbinded [Patch 5/6] > - Using more friendly wait method for reset gpio [Patch 2/6] > - Support multiple input clocks [Patch 2/6] > - Add Rob Herring's ack for DT changes > - Add Joshua Clayton's Tested-by > > Changes for v3: > - Delete "power-sequence" property at binding-doc, and change related code > at both library and user code. > - Change binding-doc example node name with Rob's comments > - of_get_named_gpio_flags only gets the gpio, but without setting gpio flags, > add additional code request gpio with proper gpio flags > - Add Philipp Zabel's Ack and MAINTAINER's entry > > Changes for v2: > - Delete "pwrseq" prefix and clock-names for properties at dt binding > - Should use structure not but its pointer for kzalloc > - Since chipidea core has no of_node, let core's of_node equals glue > layer's at core's probe > > Joshua Clayton (2): > ARM: dts: imx6qdl: Enable usb node children with > ARM: dts: imx6q-evi: Fix onboard hub reset line > > Peter Chen (6): > bindin
[PATCH] usb: musb: fix runtime PM in debugfs
MUSB driver now has runtime PM support, but the debugfs driver misses
the PM _get/_put() calls, which could cause MUSB register access
failure.
Cc: sta...@vger.kernel.org # 4.9+
Signed-off-by: Bin Liu
---
drivers/usb/musb/musb_debugfs.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
index 4d6c50549de3..7b2fd7a4c492 100644
--- a/drivers/usb/musb/musb_debugfs.c
+++ b/drivers/usb/musb/musb_debugfs.c
@@ -114,6 +114,7 @@ static int musb_regdump_show(struct seq_file *s, void
*unused)
unsignedi;
seq_printf(s, "MUSB (M)HDRC Register Dump\n");
+ pm_runtime_get_sync(musb->controller);
for (i = 0; i < ARRAY_SIZE(musb_regmap); i++) {
switch (musb_regmap[i].size) {
@@ -132,6 +133,8 @@ static int musb_regdump_show(struct seq_file *s, void
*unused)
}
}
+ pm_runtime_mark_last_busy(musb->controller);
+ pm_runtime_put_autosuspend(musb->controller);
return 0;
}
@@ -145,7 +148,10 @@ static int musb_test_mode_show(struct seq_file *s, void
*unused)
struct musb *musb = s->private;
unsignedtest;
+ pm_runtime_get_sync(musb->controller);
test = musb_readb(musb->mregs, MUSB_TESTMODE);
+ pm_runtime_mark_last_busy(musb->controller);
+ pm_runtime_put_autosuspend(musb->controller);
if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
seq_printf(s, "force host full-speed\n");
@@ -200,11 +206,12 @@ static ssize_t musb_test_mode_write(struct file *file,
u8 test;
charbuf[24];
+ pm_runtime_get_sync(musb->controller);
test = musb_readb(musb->mregs, MUSB_TESTMODE);
if (test) {
dev_err(musb->controller, "Error: test mode is already set. "
"Please do USB Bus Reset to start a new test.\n");
- return count;
+ goto ret;
}
memset(buf, 0x00, sizeof(buf));
@@ -246,6 +253,9 @@ static ssize_t musb_test_mode_write(struct file *file,
musb_writeb(musb->mregs, MUSB_TESTMODE, test);
+ret:
+ pm_runtime_mark_last_busy(musb->controller);
+ pm_runtime_put_autosuspend(musb->controller);
return count;
}
@@ -266,8 +276,13 @@ static int musb_softconnect_show(struct seq_file *s, void
*unused)
switch (musb->xceiv->otg->state) {
case OTG_STATE_A_HOST:
case OTG_STATE_A_WAIT_BCON:
+ pm_runtime_get_sync(musb->controller);
+
reg = musb_readb(musb->mregs, MUSB_DEVCTL);
connect = reg & MUSB_DEVCTL_SESSION ? 1 : 0;
+
+ pm_runtime_mark_last_busy(musb->controller);
+ pm_runtime_put_autosuspend(musb->controller);
break;
default:
connect = -1;
@@ -296,6 +311,7 @@ static ssize_t musb_softconnect_write(struct file *file,
if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
return -EFAULT;
+ pm_runtime_get_sync(musb->controller);
if (!strncmp(buf, "0", 1)) {
switch (musb->xceiv->otg->state) {
case OTG_STATE_A_HOST:
@@ -326,6 +342,8 @@ static ssize_t musb_softconnect_write(struct file *file,
}
}
+ pm_runtime_mark_last_busy(musb->controller);
+ pm_runtime_put_autosuspend(musb->controller);
return count;
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] usb: musb: debugfs: allow forcing host mode together with speed in testmode
On Mon, Dec 19, 2016 at 04:23:47PM +0100, Pali Rohár wrote:
> Based on the musb ug, force_host bit is allowed to be set along with
> force_hs or force_fs bit.
>
> It could help to implement forced host mode via testmode on Nokia N900.
>
> Signed-off-by: Pali Rohár
Applied. Thanks.
-Bin.
> ---
> Changes in v2:
> * Use == instead of & for comparison of testmode
> Changes in v3:
> * Increase buf[] to 24 bytes
> ---
> drivers/usb/musb/musb_debugfs.c | 46
> ---
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c
> index 9b22d94..4d6c505 100644
> --- a/drivers/usb/musb/musb_debugfs.c
> +++ b/drivers/usb/musb/musb_debugfs.c
> @@ -147,28 +147,34 @@ static int musb_test_mode_show(struct seq_file *s, void
> *unused)
>
> test = musb_readb(musb->mregs, MUSB_TESTMODE);
>
> - if (test & MUSB_TEST_FORCE_HOST)
> + if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS))
> + seq_printf(s, "force host full-speed\n");
> +
> + else if (test == (MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS))
> + seq_printf(s, "force host high-speed\n");
> +
> + else if (test == MUSB_TEST_FORCE_HOST)
> seq_printf(s, "force host\n");
>
> - if (test & MUSB_TEST_FIFO_ACCESS)
> + else if (test == MUSB_TEST_FIFO_ACCESS)
> seq_printf(s, "fifo access\n");
>
> - if (test & MUSB_TEST_FORCE_FS)
> + else if (test == MUSB_TEST_FORCE_FS)
> seq_printf(s, "force full-speed\n");
>
> - if (test & MUSB_TEST_FORCE_HS)
> + else if (test == MUSB_TEST_FORCE_HS)
> seq_printf(s, "force high-speed\n");
>
> - if (test & MUSB_TEST_PACKET)
> + else if (test == MUSB_TEST_PACKET)
> seq_printf(s, "test packet\n");
>
> - if (test & MUSB_TEST_K)
> + else if (test == MUSB_TEST_K)
> seq_printf(s, "test K\n");
>
> - if (test & MUSB_TEST_J)
> + else if (test == MUSB_TEST_J)
> seq_printf(s, "test J\n");
>
> - if (test & MUSB_TEST_SE0_NAK)
> + else if (test == MUSB_TEST_SE0_NAK)
> seq_printf(s, "test SE0 NAK\n");
>
> return 0;
> @@ -192,7 +198,7 @@ static ssize_t musb_test_mode_write(struct file *file,
> struct seq_file *s = file->private_data;
> struct musb *musb = s->private;
> u8 test;
> - charbuf[18];
> + charbuf[24];
>
> test = musb_readb(musb->mregs, MUSB_TESTMODE);
> if (test) {
> @@ -206,30 +212,36 @@ static ssize_t musb_test_mode_write(struct file *file,
> if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> return -EFAULT;
>
> - if (strstarts(buf, "force host"))
> + if (strstarts(buf, "force host full-speed"))
> + test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_FS;
> +
> + else if (strstarts(buf, "force host high-speed"))
> + test = MUSB_TEST_FORCE_HOST | MUSB_TEST_FORCE_HS;
> +
> + else if (strstarts(buf, "force host"))
> test = MUSB_TEST_FORCE_HOST;
>
> - if (strstarts(buf, "fifo access"))
> + else if (strstarts(buf, "fifo access"))
> test = MUSB_TEST_FIFO_ACCESS;
>
> - if (strstarts(buf, "force full-speed"))
> + else if (strstarts(buf, "force full-speed"))
> test = MUSB_TEST_FORCE_FS;
>
> - if (strstarts(buf, "force high-speed"))
> + else if (strstarts(buf, "force high-speed"))
> test = MUSB_TEST_FORCE_HS;
>
> - if (strstarts(buf, "test packet")) {
> + else if (strstarts(buf, "test packet")) {
> test = MUSB_TEST_PACKET;
> musb_load_testpacket(musb);
> }
>
> - if (strstarts(buf, "test K"))
> + else if (strstarts(buf, "test K"))
> test = MUSB_TEST_K;
>
> - if (strstarts(buf, "test J"))
> + else if (strstarts(buf, "test J"))
> test = MUSB_TEST_J;
>
> - if (strstarts(buf, "test SE0 NAK"))
> + else if (strstarts(buf, "test SE0 NAK"))
> test = MUSB_TEST_SE0_NAK;
>
> musb_writeb(musb->mregs, MUSB_TESTMODE, test);
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: musb: blackfin: fix unused warnings on suspend/resume
On Fri, Dec 16, 2016 at 07:19:39PM -0500, Jérémy Lefaure wrote:
> When CONFIG_PM_SLEEP is disabled, SIMPLE_DEV_PM_OPS does not use
> bfin_resume and bfin_suspend even if CONFIG_PM is enabled:
>
> drivers/usb/musb/blackfin.c:602:12: warning: ‘bfin_resume’ defined but
> not used [-Wunused-function]
> static int bfin_resume(struct device *dev)
> ^~~
> drivers/usb/musb/blackfin.c:585:12: warning: ‘bfin_suspend’ defined but
> not used [-Wunused-function]
> static int bfin_suspend(struct device *dev)
> ^~~~
>
> The preprocessor condition should be on CONFIG_PM_SLEEP, not on CONFIG_PM.
> However it is better to mark these functions as __maybe_unused.
Based on discussion in [1], __may_be_unused adds unnecessary image size,
right?
[1] http://www.spinics.net/lists/linux-usb/msg149994.html
Regards,
-Bin.
>
> Signed-off-by: Jérémy Lefaure
> ---
> drivers/usb/musb/blackfin.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
> index 310238c6b5cd..f43edc43268b 100644
> --- a/drivers/usb/musb/blackfin.c
> +++ b/drivers/usb/musb/blackfin.c
> @@ -580,8 +580,7 @@ static int bfin_remove(struct platform_device *pdev)
> return 0;
> }
>
> -#ifdef CONFIG_PM
> -static int bfin_suspend(struct device *dev)
> +static int __maybe_unused bfin_suspend(struct device *dev)
> {
> struct bfin_glue*glue = dev_get_drvdata(dev);
> struct musb *musb = glue_to_musb(glue);
> @@ -598,7 +597,7 @@ static int bfin_suspend(struct device *dev)
> return 0;
> }
>
> -static int bfin_resume(struct device *dev)
> +static int __maybe_unused bfin_resume(struct device *dev)
> {
> struct bfin_glue*glue = dev_get_drvdata(dev);
> struct musb *musb = glue_to_musb(glue);
> @@ -607,7 +606,6 @@ static int bfin_resume(struct device *dev)
>
> return 0;
> }
> -#endif
>
> static SIMPLE_DEV_PM_OPS(bfin_pm_ops, bfin_suspend, bfin_resume);
>
> --
> 2.11.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: musb: blackfin: add bfin_fifo_offset in bfin_ops
On Fri, Dec 16, 2016 at 07:19:40PM -0500, Jérémy Lefaure wrote:
> Commit cc92f6818f6e ("usb: musb: Populate new IO functions for
> blackfin") defines bfin_fifo_offset function without using it:
Please use 'Fixes:' comment style.
Regards,
-Bin.
>
> drivers/usb/musb/blackfin.c:36:12: warning: ‘bfin_fifo_offset’ defined
> but not used [-Wunused-function]
> static u32 bfin_fifo_offset(u8 epnum)
> ^~~~
>
> Adding bfin_fifo_offset to bfin_ops fixes this warning and allows musb
> core to call this function instead of default_fifo_offset.
>
> Signed-off-by: Jérémy Lefaure
> ---
> drivers/usb/musb/blackfin.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
> index f43edc43268b..4418574a36a1 100644
> --- a/drivers/usb/musb/blackfin.c
> +++ b/drivers/usb/musb/blackfin.c
> @@ -469,6 +469,7 @@ static const struct musb_platform_ops bfin_ops = {
> .init = bfin_musb_init,
> .exit = bfin_musb_exit,
>
> + .fifo_offset= bfin_fifo_offset,
> .readb = bfin_readb,
> .writeb = bfin_writeb,
> .readw = bfin_readw,
> --
> 2.11.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/13] USB: serial: ch341: fix initial line settings
>> Apart from the two additional tests mentioned above, can you also
>> provide a log from when connecting the device using the following commit
>> that I just pushed to the ch341 branch:
>>
>> f341ee36198d ("dbg: ch341: add register dumps to probe")
>>
>> which provides dumps of the register settings during initialisation.
>> (Make sure ch341 dynamic debugging is not enabled to avoid cluttering
>> the log.)
>
> I'll send this in a followup, need to rebuild.
00018-gf341ee3:
Dec 19 13:51:13 willard kernel: usbcore: registered new interface driver ch341
Dec 19 13:51:13 willard kernel: usbserial: USB Serial support
registered for ch341-uart
Dec 19 13:51:23 willard kernel: usb 6-2: new full-speed USB device
number 10 using uhci_hcd
Dec 19 13:51:23 willard kernel: usb 6-2: New USB device found,
idVendor=1a86, idProduct=7523
Dec 19 13:51:23 willard kernel: usb 6-2: New USB device strings:
Mfr=0, Product=2, SerialNumber=0
Dec 19 13:51:23 willard kernel: usb 6-2: Product: USB2.0-Ser!
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: ch341-uart converter detected
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [00] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [01] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [02] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [03] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [04] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [05] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [06] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [07] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [08] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [09] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0a] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0b] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0c] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0d] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0e] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0f] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [10] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [11] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [12] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [13] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [14] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [15] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [16] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [17] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [18] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [19] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1a] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1b] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1c] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1d] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1e] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [1f] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [20] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [21] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [22] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [23] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [24] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [25] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [26] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [27] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [28] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [29] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2a] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2b] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2c] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2d] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2e] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [2f] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: init 0 0
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [00] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [01] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [02] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [03] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [04] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [05] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [06] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [07] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [08] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [09] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0a] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0b] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0c] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0d] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0e] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [0f] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [10] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [11] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0: [12] = 00
Dec 19 13:51:23 willard kernel: ch341 6-2:1.0:
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 19 December 2016 at 20:13, Mathias Nyman wrote: > On 19.12.2016 13:34, Baolin Wang wrote: >> >> Hi Mathias, >> >> On 19 December 2016 at 18:33, Mathias Nyman >> wrote: >>> >>> On 13.12.2016 05:21, Baolin Wang wrote: Hi Mathias, On 12 December 2016 at 23:52, Mathias Nyman wrote: > > > On 05.12.2016 09:51, Baolin Wang wrote: >> >> >> >> If a command event is found on the event ring during an interrupt, >> we need to stop the command timer with del_timer(). Since del_timer() >> can fail if the timer is running and waiting on the xHCI lock, then >> it maybe get the wrong timeout command in >> xhci_handle_command_timeout() >> if host fetched a new command and updated the xhci->current_cmd in >> handle_cmd_completion(). For this situation, we need a way to signal >> to the command timer that everything is fine and it should exit. > > > > > Ah, right, this could actually happen. > >> >> >> We should introduce a counter (xhci->current_cmd_pending) for the >> number >> of pending commands. If we need to cancel the command timer and >> del_timer() >> succeeds, we decrement the number of pending commands. If del_timer() >> fails, >> we leave the number of pending commands alone. >> >> For handling timeout command, in xhci_handle_command_timeout() we will >> check >> the counter after decrementing it, if the counter >> (xhci->current_cmd_pending) >> is 0, which means xhci->current_cmd is the right timeout command. If >> the >> counter (xhci->current_cmd_pending) is greater than 0, which means >> current >> timeout command has been handled by host and host has fetched new >> command >> as >> xhci->current_cmd, then just return and wait for new current command. > > > > > A counter like this could work. > > Writing the abort bit can generate either ABORT+STOP, or just STOP > event, this seems to cover both. > > quick check, case 1: timeout and cmd completion at the same time. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > --completion irq fires---- timer times out at same > time-- > handle_cmd_completion() handle_cmd_timeout(),) > lock(xhci_lock ) spin_on(xhci_lock) > del_timer() fail, p (=1, nochange) > cur_cmd = list_next(), p++ (=2) > unlock(xhci_lock) > lock(xhci_lock) > p-- (=1) > if (p > 0), exit > OK works > > case 2: normal timeout case with ABORT+STOP, no race. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > handle_cmd_timeout() > p-- (P=0), don't exit > mod_timer(), p++ (P=1) > write_abort_bit() > handle_cmd_comletion(ABORT) > del_timer(), ok, p-- (p = 0) > handle_cmd_completion(STOP) > del_timer(), fail, (P=0) > handle_stopped_cmd_ring() > cur_cmd = list_next(), p++ (=1) > mod_timer() > > OK, works, and same for just STOP case, with the only difference that > during handle_cmd_completion(STOP) p is decremented (p--) Yes, that's the cases what I want to handle, thanks for your explicit explanation. >>> >>> Gave this some more thought over the weekend, and this implementation >>> doesn't solve the case when the last command times out and races with the >>> completion handler: >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> --completion irq fires---- timer times out at same time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock )spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> no more commands, P (=1, nochange) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=0) >>> p == 0, continue, even if we >>> should >>> not. >>>For this we still need to rely >>> on >>> checking cur_cmd == NULL in the timeout function. >>> (Baolus patch sets it to NULL if there are no more commands pending) >> >> >> As I pointed out in patch 1 of this patchset, this patchset is based >> on Lu Baolu's new fix patch: >> usb: xhci: fix possible wild pointer >> https://www.spinics.net/lists/linux-usb/msg150219.html >> >> After applying Baolu's patch, after decrement
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi Mathias, On 12/19/2016 08:13 PM, Mathias Nyman wrote: > On 19.12.2016 13:34, Baolin Wang wrote: >> Hi Mathias, >> >> On 19 December 2016 at 18:33, Mathias Nyman >> wrote: >>> On 13.12.2016 05:21, Baolin Wang wrote: Hi Mathias, On 12 December 2016 at 23:52, Mathias Nyman wrote: > > On 05.12.2016 09:51, Baolin Wang wrote: >> >> >> If a command event is found on the event ring during an interrupt, >> we need to stop the command timer with del_timer(). Since del_timer() >> can fail if the timer is running and waiting on the xHCI lock, then >> it maybe get the wrong timeout command in xhci_handle_command_timeout() >> if host fetched a new command and updated the xhci->current_cmd in >> handle_cmd_completion(). For this situation, we need a way to signal >> to the command timer that everything is fine and it should exit. > > > > Ah, right, this could actually happen. > >> >> >> We should introduce a counter (xhci->current_cmd_pending) for the number >> of pending commands. If we need to cancel the command timer and >> del_timer() >> succeeds, we decrement the number of pending commands. If del_timer() >> fails, >> we leave the number of pending commands alone. >> >> For handling timeout command, in xhci_handle_command_timeout() we will >> check >> the counter after decrementing it, if the counter >> (xhci->current_cmd_pending) >> is 0, which means xhci->current_cmd is the right timeout command. If the >> counter (xhci->current_cmd_pending) is greater than 0, which means >> current >> timeout command has been handled by host and host has fetched new >> command >> as >> xhci->current_cmd, then just return and wait for new current command. > > > > A counter like this could work. > > Writing the abort bit can generate either ABORT+STOP, or just STOP > event, this seems to cover both. > > quick check, case 1: timeout and cmd completion at the same time. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > --completion irq fires---- timer times out at same time-- > handle_cmd_completion() handle_cmd_timeout(),) > lock(xhci_lock ) spin_on(xhci_lock) > del_timer() fail, p (=1, nochange) > cur_cmd = list_next(), p++ (=2) > unlock(xhci_lock) > lock(xhci_lock) > p-- (=1) > if (p > 0), exit > OK works > > case 2: normal timeout case with ABORT+STOP, no race. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > handle_cmd_timeout() > p-- (P=0), don't exit > mod_timer(), p++ (P=1) > write_abort_bit() > handle_cmd_comletion(ABORT) > del_timer(), ok, p-- (p = 0) > handle_cmd_completion(STOP) > del_timer(), fail, (P=0) > handle_stopped_cmd_ring() > cur_cmd = list_next(), p++ (=1) > mod_timer() > > OK, works, and same for just STOP case, with the only difference that > during handle_cmd_completion(STOP) p is decremented (p--) Yes, that's the cases what I want to handle, thanks for your explicit explanation. >>> >>> Gave this some more thought over the weekend, and this implementation >>> doesn't solve the case when the last command times out and races with the >>> completion handler: >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> --completion irq fires---- timer times out at same time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock )spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> no more commands, P (=1, nochange) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=0) >>> p == 0, continue, even if we should >>> not. >>>For this we still need to rely on >>> checking cur_cmd == NULL in the timeout function. >>> (Baolus patch sets it to NULL if there are no more commands pending) >> >> As I pointed out in patch 1 of this patchset, this patchset is based >> on Lu Baolu's new fix patch: >> usb: xhci: fix possible wild pointer >> https://www.spinics.net/lists/linux-usb/msg150219.html >> >> After applying Baolu's patch, after decrement the counter, we will >> check the xhci->cur_command if is NULL. So in thi
Re: [PATCH v10 0/8] power: add power sequence library
On Mon, Dec 19, 2016 at 09:15:04PM +0200, Krzysztof Kozlowski wrote: > On Mon, Nov 14, 2016 at 09:35:51AM +0800, Peter Chen wrote: > > Hi all, > > > > This is a follow-up for my last power sequence framework patch set [1]. > > According to Rob Herring and Ulf Hansson's comments[2]. The kinds of > > power sequence instances will be added at postcore_initcall, the match > > criteria is compatible string first, if the compatible string is not > > matched between dts and library, it will try to use generic power sequence. > > > > The host driver just needs to call of_pwrseq_on/of_pwrseq_off > > if only one power sequence instance is needed, for more power sequences > > are used, using of_pwrseq_on_list/of_pwrseq_off_list instead (eg, USB hub > > driver). > > > > In future, if there are special power sequence requirements, the special > > power sequence library can be created. > > > > This patch set is tested on i.mx6 sabresx evk using a dts change, I use > > two hot-plug devices to simulate this use case, the related binding > > change is updated at patch [1/6], The udoo board changes were tested > > using my last power sequence patch set.[3] > > > > Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also > > need to power on itself before it can be found by ULPI bus. > > > > [1] http://www.spinics.net/lists/linux-usb/msg142755.html > > [2] http://www.spinics.net/lists/linux-usb/msg143106.html > > [3] http://www.spinics.net/lists/linux-usb/msg142815.html > > > > Changes for v10: > > - Improve the kernel-doc for power sequence core, including exported APIs > > and > > main structure. [Patch 2/8] > > - Change Kconfig, and let the user choose power sequence. [Patch 2/8] > > - Delete EXPORT_SYMBOL and change related APIs as local, these APIs do not > > be intended to export currently. [Patch 2/8] > > - Selete POWER_SEQUENCE at USB core's Kconfig. [Patch 4/8] > > Hi Peter, > > It is great that you continued the work on this. > > I saw (in some previous mails) your repo mentioned: > https://git.kernel.org/cgit/linux/kernel/git/peter.chen/usb.git/ > Does it contain the recent version of this patchset? > > I want to build on top of it fixes for usb3503 on Odroid U3 board. Krzysztof, I put v10 patch set at branch: pwrseq-lib. It seems there are no more comments I will send my v11 patch set after new year holiday. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[peter.chen-usb:pwrseq-lib 4/9] warning: (USB) selects POWER_SEQUENCE which has unmet direct dependencies (OF)
tree: https://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git pwrseq-lib head: 284f898fe1c6ea79103391c1b1384694567732c2 commit: 1b4828637fb0c5b4ac844290a3127bf8769f9f61 [4/9] usb: core: add power sequence handling for USB devices config: x86_64-randconfig-x006-201651 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: git checkout 1b4828637fb0c5b4ac844290a3127bf8769f9f61 # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): warning: (USB) selects POWER_SEQUENCE which has unmet direct dependencies (OF) --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH v2 2/2] usb: host: xhci: Handle the right timeout command
If a command event is found on the event ring during an interrupt,
we need to stop the command timer with del_timer(). Since del_timer()
can fail if the timer is running and waiting on the xHCI lock, then
it maybe get the wrong timeout command in xhci_handle_command_timeout()
if host fetched a new command and updated the xhci->current_cmd in
handle_cmd_completion(). For this situation, we need a way to signal
to the command timer that everything is fine and it should exit.
We should check if the command timer is pending in xhci_handle_command_timeout()
function, if the command timer is pending, which means current timeout
command has been handled by host and host has fetched new command and
re-added the command timer, then just return and wait for new current
command. If not, it means current command is timeout and need to be
handled.
Signed-off-by: Baolin Wang
---
Changes since v1:
- Remove the counter and just check if the command timer is pending.
---
drivers/usb/host/xhci-ring.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 9965a4c..3947344 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1269,7 +1269,7 @@ void xhci_handle_command_timeout(unsigned long data)
xhci = (struct xhci_hcd *) data;
spin_lock_irqsave(&xhci->lock, flags);
- if (!xhci->current_cmd) {
+ if (!xhci->current_cmd || timer_pending(&xhci->cmd_timer)) {
spin_unlock_irqrestore(&xhci->lock, flags);
return;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 20 December 2016 at 12:29, Lu Baolu wrote: > Hi Mathias, > > On 12/19/2016 08:13 PM, Mathias Nyman wrote: >> On 19.12.2016 13:34, Baolin Wang wrote: >>> Hi Mathias, >>> >>> On 19 December 2016 at 18:33, Mathias Nyman >>> wrote: On 13.12.2016 05:21, Baolin Wang wrote: > > Hi Mathias, > > On 12 December 2016 at 23:52, Mathias Nyman > wrote: >> >> On 05.12.2016 09:51, Baolin Wang wrote: >>> >>> >>> If a command event is found on the event ring during an interrupt, >>> we need to stop the command timer with del_timer(). Since del_timer() >>> can fail if the timer is running and waiting on the xHCI lock, then >>> it maybe get the wrong timeout command in xhci_handle_command_timeout() >>> if host fetched a new command and updated the xhci->current_cmd in >>> handle_cmd_completion(). For this situation, we need a way to signal >>> to the command timer that everything is fine and it should exit. >> >> >> >> Ah, right, this could actually happen. >> >>> >>> >>> We should introduce a counter (xhci->current_cmd_pending) for the number >>> of pending commands. If we need to cancel the command timer and >>> del_timer() >>> succeeds, we decrement the number of pending commands. If del_timer() >>> fails, >>> we leave the number of pending commands alone. >>> >>> For handling timeout command, in xhci_handle_command_timeout() we will >>> check >>> the counter after decrementing it, if the counter >>> (xhci->current_cmd_pending) >>> is 0, which means xhci->current_cmd is the right timeout command. If the >>> counter (xhci->current_cmd_pending) is greater than 0, which means >>> current >>> timeout command has been handled by host and host has fetched new >>> command >>> as >>> xhci->current_cmd, then just return and wait for new current command. >> >> >> >> A counter like this could work. >> >> Writing the abort bit can generate either ABORT+STOP, or just STOP >> event, this seems to cover both. >> >> quick check, case 1: timeout and cmd completion at the same time. >> >> cpu1cpu2 >> >> queue_command(first), p++ (=1) >> queue_command(more), >> --completion irq fires---- timer times out at same time-- >> handle_cmd_completion() handle_cmd_timeout(),) >> lock(xhci_lock ) spin_on(xhci_lock) >> del_timer() fail, p (=1, nochange) >> cur_cmd = list_next(), p++ (=2) >> unlock(xhci_lock) >> lock(xhci_lock) >> p-- (=1) >> if (p > 0), exit >> OK works >> >> case 2: normal timeout case with ABORT+STOP, no race. >> >> cpu1cpu2 >> >> queue_command(first), p++ (=1) >> queue_command(more), >> handle_cmd_timeout() >> p-- (P=0), don't exit >> mod_timer(), p++ (P=1) >> write_abort_bit() >> handle_cmd_comletion(ABORT) >> del_timer(), ok, p-- (p = 0) >> handle_cmd_completion(STOP) >> del_timer(), fail, (P=0) >> handle_stopped_cmd_ring() >> cur_cmd = list_next(), p++ (=1) >> mod_timer() >> >> OK, works, and same for just STOP case, with the only difference that >> during handle_cmd_completion(STOP) p is decremented (p--) > > > Yes, that's the cases what I want to handle, thanks for your explicit > explanation. > Gave this some more thought over the weekend, and this implementation doesn't solve the case when the last command times out and races with the completion handler: cpu1cpu2 queue_command(first), p++ (=1) --completion irq fires---- timer times out at same time-- handle_cmd_completion() handle_cmd_timeout(),) lock(xhci_lock )spin_on(xhci_lock) del_timer() fail, p (=1, nochange) no more commands, P (=1, nochange) unlock(xhci_lock) lock(xhci_lock) p-- (=0) p == 0, continue, even if we should not. For this we still need to rely on checking cur_cmd == NULL in the timeout function. (Baolus patch sets it to NULL if there are no more commands pending) >>> >>> As I pointed out in patch 1 of this patchset, this patchset is based >>> on Lu Baolu's new fix patch: >>> usb: xhci: fix possible wild
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 12/20/2016 02:06 PM, Baolin Wang wrote: > Hi, > > On 20 December 2016 at 12:29, Lu Baolu wrote: >> Hi Mathias, >> >> On 12/19/2016 08:13 PM, Mathias Nyman wrote: >>> On 19.12.2016 13:34, Baolin Wang wrote: Hi Mathias, On 19 December 2016 at 18:33, Mathias Nyman wrote: > On 13.12.2016 05:21, Baolin Wang wrote: >> Hi Mathias, >> >> On 12 December 2016 at 23:52, Mathias Nyman >> wrote: >>> On 05.12.2016 09:51, Baolin Wang wrote: If a command event is found on the event ring during an interrupt, we need to stop the command timer with del_timer(). Since del_timer() can fail if the timer is running and waiting on the xHCI lock, then it maybe get the wrong timeout command in xhci_handle_command_timeout() if host fetched a new command and updated the xhci->current_cmd in handle_cmd_completion(). For this situation, we need a way to signal to the command timer that everything is fine and it should exit. >>> >>> >>> Ah, right, this could actually happen. >>> We should introduce a counter (xhci->current_cmd_pending) for the number of pending commands. If we need to cancel the command timer and del_timer() succeeds, we decrement the number of pending commands. If del_timer() fails, we leave the number of pending commands alone. For handling timeout command, in xhci_handle_command_timeout() we will check the counter after decrementing it, if the counter (xhci->current_cmd_pending) is 0, which means xhci->current_cmd is the right timeout command. If the counter (xhci->current_cmd_pending) is greater than 0, which means current timeout command has been handled by host and host has fetched new command as xhci->current_cmd, then just return and wait for new current command. >>> >>> >>> A counter like this could work. >>> >>> Writing the abort bit can generate either ABORT+STOP, or just STOP >>> event, this seems to cover both. >>> >>> quick check, case 1: timeout and cmd completion at the same time. >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> queue_command(more), >>> --completion irq fires---- timer times out at same >>> time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock ) spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> cur_cmd = list_next(), p++ (=2) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=1) >>> if (p > 0), exit >>> OK works >>> >>> case 2: normal timeout case with ABORT+STOP, no race. >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> queue_command(more), >>> handle_cmd_timeout() >>> p-- (P=0), don't exit >>> mod_timer(), p++ (P=1) >>> write_abort_bit() >>> handle_cmd_comletion(ABORT) >>> del_timer(), ok, p-- (p = 0) >>> handle_cmd_completion(STOP) >>> del_timer(), fail, (P=0) >>> handle_stopped_cmd_ring() >>> cur_cmd = list_next(), p++ (=1) >>> mod_timer() >>> >>> OK, works, and same for just STOP case, with the only difference that >>> during handle_cmd_completion(STOP) p is decremented (p--) >> >> Yes, that's the cases what I want to handle, thanks for your explicit >> explanation. >> > Gave this some more thought over the weekend, and this implementation > doesn't solve the case when the last command times out and races with the > completion handler: > > cpu1cpu2 > > queue_command(first), p++ (=1) > --completion irq fires---- timer times out at same time-- > handle_cmd_completion() handle_cmd_timeout(),) > lock(xhci_lock )spin_on(xhci_lock) > del_timer() fail, p (=1, nochange) > no more commands, P (=1, nochange) > unlock(xhci_lock) > lock(xhci_lock) > p-- (=0) > p == 0, continue, even if we > should > not. >For this we still need to rely > on > checking cur_cmd == NULL in the timeout function. > (Baolus patch sets it to NULL if there are no more commands pending)
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
On 20 December 2016 at 14:39, Lu Baolu wrote: > Hi, > > On 12/20/2016 02:06 PM, Baolin Wang wrote: >> Hi, >> >> On 20 December 2016 at 12:29, Lu Baolu wrote: >>> Hi Mathias, >>> >>> On 12/19/2016 08:13 PM, Mathias Nyman wrote: On 19.12.2016 13:34, Baolin Wang wrote: > Hi Mathias, > > On 19 December 2016 at 18:33, Mathias Nyman > wrote: >> On 13.12.2016 05:21, Baolin Wang wrote: >>> Hi Mathias, >>> >>> On 12 December 2016 at 23:52, Mathias Nyman >>> wrote: On 05.12.2016 09:51, Baolin Wang wrote: > > If a command event is found on the event ring during an interrupt, > we need to stop the command timer with del_timer(). Since del_timer() > can fail if the timer is running and waiting on the xHCI lock, then > it maybe get the wrong timeout command in > xhci_handle_command_timeout() > if host fetched a new command and updated the xhci->current_cmd in > handle_cmd_completion(). For this situation, we need a way to signal > to the command timer that everything is fine and it should exit. Ah, right, this could actually happen. > > We should introduce a counter (xhci->current_cmd_pending) for the > number > of pending commands. If we need to cancel the command timer and > del_timer() > succeeds, we decrement the number of pending commands. If del_timer() > fails, > we leave the number of pending commands alone. > > For handling timeout command, in xhci_handle_command_timeout() we will > check > the counter after decrementing it, if the counter > (xhci->current_cmd_pending) > is 0, which means xhci->current_cmd is the right timeout command. If > the > counter (xhci->current_cmd_pending) is greater than 0, which means > current > timeout command has been handled by host and host has fetched new > command > as > xhci->current_cmd, then just return and wait for new current command. A counter like this could work. Writing the abort bit can generate either ABORT+STOP, or just STOP event, this seems to cover both. quick check, case 1: timeout and cmd completion at the same time. cpu1cpu2 queue_command(first), p++ (=1) queue_command(more), --completion irq fires---- timer times out at same time-- handle_cmd_completion() handle_cmd_timeout(),) lock(xhci_lock ) spin_on(xhci_lock) del_timer() fail, p (=1, nochange) cur_cmd = list_next(), p++ (=2) unlock(xhci_lock) lock(xhci_lock) p-- (=1) if (p > 0), exit OK works case 2: normal timeout case with ABORT+STOP, no race. cpu1cpu2 queue_command(first), p++ (=1) queue_command(more), handle_cmd_timeout() p-- (P=0), don't exit mod_timer(), p++ (P=1) write_abort_bit() handle_cmd_comletion(ABORT) del_timer(), ok, p-- (p = 0) handle_cmd_completion(STOP) del_timer(), fail, (P=0) handle_stopped_cmd_ring() cur_cmd = list_next(), p++ (=1) mod_timer() OK, works, and same for just STOP case, with the only difference that during handle_cmd_completion(STOP) p is decremented (p--) >>> >>> Yes, that's the cases what I want to handle, thanks for your explicit >>> explanation. >>> >> Gave this some more thought over the weekend, and this implementation >> doesn't solve the case when the last command times out and races with the >> completion handler: >> >> cpu1cpu2 >> >> queue_command(first), p++ (=1) >> --completion irq fires---- timer times out at same time-- >> handle_cmd_completion() handle_cmd_timeout(),) >> lock(xhci_lock )spin_on(xhci_lock) >> del_timer() fail, p (=1, nochange) >> no more commands, P (=1, nochange) >> unlock(xhci_lock) >> lock(xhci_lock) >> p-- (=0) >> p == 0, continue, even if we >> should >> not. >>
Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
Hi Neil, On 3 November 2016 at 09:25, NeilBrown wrote: > On Tue, Nov 01 2016, Baolin Wang wrote: > > >>> So I won't be responding on this topic any further until I see a genuine >>> attempt to understand and resolve the inconsistencies with >>> usb_register_notifier(). >> >> Any better solution? > > I'm not sure exactly what you are asking, so I'll assume you are asking > the question I want to answer :-) > > 1/ Liase with the extcon developers to resolve the inconsistencies > with USB connector types. > e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP" > which both seem to suggest a standard downstream port. There is no > documentation describing how these relate, and no consistent practice > to copy. > I suspect the intention is that > EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of > the cable, while EXTCON_CHG_USB* indicate the power capabilities of > the cable. > So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB > while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA > would normally appear with EXTCON_USB_HOST (I think). > Some drivers follow this model, particularly extcon-max14577.c > but it is not consistent. > > This policy should be well documented and possibly existing drivers > should be updated to follow it. > > At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW > and EXTCON_CHG_USB_FAST. These names don't mean much. > They were recently removed from drivers/power/axp288_charger.c > which is good, but are still used in drivers/extcon/extcon-max* > Possibly they should be changed to names from the standard, or > possibly they should be renamed to identify the current they are > expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A Now I am creating the new patchset with fixing and converting exist drivers. I did some investigation about EXTCON subsystem. From your suggestion: 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB. After checking, now all extcon drivers were following this rule. 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST. Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to change. 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A. There are no model that shows the slow charger should be 500mA and fast charger is 1A. (In extcon-max77693.c file, the fast charger can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful I think. >From above investigation, I did not find anything need to be changed now. What do you think? Thanks. > > 2/ Change all usb phys to register an extcon and to send appropriate >notifications. Many already do, but I don't think it is universal. >It is probable that the extcon should be registered using common code >instead of each phy driver having its own >extcon_get_edev_by_phandle() >or whatever. >If the usb phy driver needs to look at battery charger registers to >know what sort of cable was connected (which I believe is the case >for the chips you are interested in), then it should do that. > > 3/ Currently some USB controllers discover that a cable was connected by >listening on an extcon, and some by registering for a usb_notifier >(described below) ... though there seem to only be 2 left which do that. >Now that all USB phys send connection information via extcon (see 2), >the USB controllers should be changed to all find out about the cable >using extcon. > > 4/ struct usb_phy contains: > /* for notification of usb_phy_events */ > struct atomic_notifier_head notifier; > > This is used inconsistently. Sometimes the argument passed > is NULL, sometimes it is a pointer to 'vbus_draw' - the current > limited negotiated via USB, sometimes it is a pointer the the gadget > though as far as I can tell, that last one is never used. > > This should be changed to be consistent. This notifier is no longer > needed to tell the USB controller that a cable was connected (extcon > now does that, see 3) so it is only used to communicate the > 'vbus_draw' information. > So it should be changed to *only* send a notification when vbus_draw > is known, and it should carry that information. > This should probably be done in common code, and removed > from individual drivers. > > 5/ Now that all cable connection notifications are sent over extcon and >all vbus_draw notifications are sent over the usb_phy notifier, write >some support code that a power supply client can use to be told what >power is available. >e.g. a battery charger driver would call: >register_power_client(.) >or similar, providing a phandle (or similar) for the usb phy and a >function to call back when the available current changes (or maybe a >work_struct containing the function pointer) > >register_power_client() would the
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 12/20/2016 02:46 PM, Baolin Wang wrote: > On 20 December 2016 at 14:39, Lu Baolu wrote: >> Hi, >> >> On 12/20/2016 02:06 PM, Baolin Wang wrote: >>> Hi, >>> >>> On 20 December 2016 at 12:29, Lu Baolu wrote: Hi Mathias, On 12/19/2016 08:13 PM, Mathias Nyman wrote: > On 19.12.2016 13:34, Baolin Wang wrote: >> Hi Mathias, >> >> On 19 December 2016 at 18:33, Mathias Nyman >> wrote: >>> On 13.12.2016 05:21, Baolin Wang wrote: Hi Mathias, On 12 December 2016 at 23:52, Mathias Nyman wrote: > On 05.12.2016 09:51, Baolin Wang wrote: >> If a command event is found on the event ring during an interrupt, >> we need to stop the command timer with del_timer(). Since del_timer() >> can fail if the timer is running and waiting on the xHCI lock, then >> it maybe get the wrong timeout command in >> xhci_handle_command_timeout() >> if host fetched a new command and updated the xhci->current_cmd in >> handle_cmd_completion(). For this situation, we need a way to signal >> to the command timer that everything is fine and it should exit. > > Ah, right, this could actually happen. > >> We should introduce a counter (xhci->current_cmd_pending) for the >> number >> of pending commands. If we need to cancel the command timer and >> del_timer() >> succeeds, we decrement the number of pending commands. If del_timer() >> fails, >> we leave the number of pending commands alone. >> >> For handling timeout command, in xhci_handle_command_timeout() we >> will >> check >> the counter after decrementing it, if the counter >> (xhci->current_cmd_pending) >> is 0, which means xhci->current_cmd is the right timeout command. If >> the >> counter (xhci->current_cmd_pending) is greater than 0, which means >> current >> timeout command has been handled by host and host has fetched new >> command >> as >> xhci->current_cmd, then just return and wait for new current command. > > A counter like this could work. > > Writing the abort bit can generate either ABORT+STOP, or just STOP > event, this seems to cover both. > > quick check, case 1: timeout and cmd completion at the same time. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > --completion irq fires---- timer times out at same > time-- > handle_cmd_completion() handle_cmd_timeout(),) > lock(xhci_lock ) spin_on(xhci_lock) > del_timer() fail, p (=1, nochange) > cur_cmd = list_next(), p++ (=2) > unlock(xhci_lock) > lock(xhci_lock) > p-- (=1) > if (p > 0), exit > OK works > > case 2: normal timeout case with ABORT+STOP, no race. > > cpu1cpu2 > > queue_command(first), p++ (=1) > queue_command(more), > handle_cmd_timeout() > p-- (P=0), don't exit > mod_timer(), p++ (P=1) > write_abort_bit() > handle_cmd_comletion(ABORT) > del_timer(), ok, p-- (p = 0) > handle_cmd_completion(STOP) > del_timer(), fail, (P=0) > handle_stopped_cmd_ring() > cur_cmd = list_next(), p++ (=1) > mod_timer() > > OK, works, and same for just STOP case, with the only difference that > during handle_cmd_completion(STOP) p is decremented (p--) Yes, that's the cases what I want to handle, thanks for your explicit explanation. >>> Gave this some more thought over the weekend, and this implementation >>> doesn't solve the case when the last command times out and races with >>> the >>> completion handler: >>> >>> cpu1cpu2 >>> >>> queue_command(first), p++ (=1) >>> --completion irq fires---- timer times out at same >>> time-- >>> handle_cmd_completion() handle_cmd_timeout(),) >>> lock(xhci_lock )spin_on(xhci_lock) >>> del_timer() fail, p (=1, nochange) >>> no more commands, P (=1, nochange) >>> unlock(xhci_lock) >>> lock(xhci_lock) >>> p-- (=0
Re: [PATCH 2/2] usb: host: xhci: Handle the right timeout command
Hi, On 20 December 2016 at 15:18, Lu Baolu wrote: > Hi, > > On 12/20/2016 02:46 PM, Baolin Wang wrote: >> On 20 December 2016 at 14:39, Lu Baolu wrote: >>> Hi, >>> >>> On 12/20/2016 02:06 PM, Baolin Wang wrote: Hi, On 20 December 2016 at 12:29, Lu Baolu wrote: > Hi Mathias, > > On 12/19/2016 08:13 PM, Mathias Nyman wrote: >> On 19.12.2016 13:34, Baolin Wang wrote: >>> Hi Mathias, >>> >>> On 19 December 2016 at 18:33, Mathias Nyman >>> wrote: On 13.12.2016 05:21, Baolin Wang wrote: > Hi Mathias, > > On 12 December 2016 at 23:52, Mathias Nyman > wrote: >> On 05.12.2016 09:51, Baolin Wang wrote: >>> If a command event is found on the event ring during an interrupt, >>> we need to stop the command timer with del_timer(). Since >>> del_timer() >>> can fail if the timer is running and waiting on the xHCI lock, then >>> it maybe get the wrong timeout command in >>> xhci_handle_command_timeout() >>> if host fetched a new command and updated the xhci->current_cmd in >>> handle_cmd_completion(). For this situation, we need a way to signal >>> to the command timer that everything is fine and it should exit. >> >> Ah, right, this could actually happen. >> >>> We should introduce a counter (xhci->current_cmd_pending) for the >>> number >>> of pending commands. If we need to cancel the command timer and >>> del_timer() >>> succeeds, we decrement the number of pending commands. If >>> del_timer() >>> fails, >>> we leave the number of pending commands alone. >>> >>> For handling timeout command, in xhci_handle_command_timeout() we >>> will >>> check >>> the counter after decrementing it, if the counter >>> (xhci->current_cmd_pending) >>> is 0, which means xhci->current_cmd is the right timeout command. >>> If the >>> counter (xhci->current_cmd_pending) is greater than 0, which means >>> current >>> timeout command has been handled by host and host has fetched new >>> command >>> as >>> xhci->current_cmd, then just return and wait for new current >>> command. >> >> A counter like this could work. >> >> Writing the abort bit can generate either ABORT+STOP, or just STOP >> event, this seems to cover both. >> >> quick check, case 1: timeout and cmd completion at the same time. >> >> cpu1cpu2 >> >> queue_command(first), p++ (=1) >> queue_command(more), >> --completion irq fires---- timer times out at same >> time-- >> handle_cmd_completion() handle_cmd_timeout(),) >> lock(xhci_lock ) spin_on(xhci_lock) >> del_timer() fail, p (=1, nochange) >> cur_cmd = list_next(), p++ (=2) >> unlock(xhci_lock) >> lock(xhci_lock) >> p-- (=1) >> if (p > 0), exit >> OK works >> >> case 2: normal timeout case with ABORT+STOP, no race. >> >> cpu1cpu2 >> >> queue_command(first), p++ (=1) >> queue_command(more), >> handle_cmd_timeout() >> p-- (P=0), don't exit >> mod_timer(), p++ (P=1) >> write_abort_bit() >> handle_cmd_comletion(ABORT) >> del_timer(), ok, p-- (p = 0) >> handle_cmd_completion(STOP) >> del_timer(), fail, (P=0) >> handle_stopped_cmd_ring() >> cur_cmd = list_next(), p++ (=1) >> mod_timer() >> >> OK, works, and same for just STOP case, with the only difference that >> during handle_cmd_completion(STOP) p is decremented (p--) > Yes, that's the cases what I want to handle, thanks for your explicit > explanation. > Gave this some more thought over the weekend, and this implementation doesn't solve the case when the last command times out and races with the completion handler: cpu1cpu2 queue_command(first), p++ (=1) --completion irq fires---- timer times out at same time-- handle_cmd_completion() handle_cmd_timeout(),) lock(xhci_lock )spin_on(xhci_lock) del_timer() fail
