Re: [PATCH] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Sun, Nov 03, 2013 at 03:26:24PM +0100, Johan Hovold wrote: > On Sun, Nov 03, 2013 at 02:39:32PM +0200, Mika Westerberg wrote: > > On Fri, Nov 01, 2013 at 08:08:31AM -0700, Greg KH wrote: > > > I'll go revert all of these for now and send it to Linus for 3.12-final, > > > and then we can start "fresh" in resolving this issue. > > > > I tested again with the reverts and now my TU-S9 USB-serial converter works > > fine. > > > > Thanks everyone. > > > > Frank, Johan, > > > > I can test your upcoming patches on my HW if needed. > > Great. Which type of pl2303 is the TU-S9? It's HXD. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Sun, Nov 03, 2013 at 02:39:32PM +0200, Mika Westerberg wrote: > On Fri, Nov 01, 2013 at 08:08:31AM -0700, Greg KH wrote: > > I'll go revert all of these for now and send it to Linus for 3.12-final, > > and then we can start "fresh" in resolving this issue. > > I tested again with the reverts and now my TU-S9 USB-serial converter works > fine. > > Thanks everyone. > > Frank, Johan, > > I can test your upcoming patches on my HW if needed. Great. Which type of pl2303 is the TU-S9? Frank, which device types do you have access to? Thanks, 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 08:08:31AM -0700, Greg KH wrote: > I'll go revert all of these for now and send it to Linus for 3.12-final, > and then we can start "fresh" in resolving this issue. I tested again with the reverts and now my TU-S9 USB-serial converter works fine. Thanks everyone. Frank, Johan, I can test your upcoming patches on my HW if needed. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 03:25:23PM +0100, Johan Hovold wrote: > On Fri, Nov 01, 2013 at 02:47:27PM +0100, Frank Schäfer wrote: > > Am 01.11.2013 14:29, schrieb Johan Hovold: > > > On Fri, Nov 01, 2013 at 01:03:24PM +0100, Frank Schäfer wrote: > > >> Am 01.11.2013 12:15, schrieb Johan Hovold: > > >>> On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: > > Am 01.11.2013 11:22, schrieb Johan Hovold: > > > I think that adding support for odd baud rates (and the improved > > > device > > > detection) can be implemented in a cleaner way and want to have a > > > chance > > > to discuss that with you. > > Ok, between the lines I can read that you just don't trust this patch > > series. ;) > > I'm looking forward to your suggestions for a cleaner way to implement > > this. :) > > >>> You have done a great job here in reverse-engineering the chip and > > >>> documenting the quirks. It's not an easy job at all with all the > > >>> different chips out there, including pirate ones. > > >>> > > >>> I just think we should take the safe way here and hold off the changes > > >>> another cycle. Get some more people to do more testing (which chip types > > >>> do we have among us at the moment?). This would also allow us to come up > > >>> with a series which abstracts the differences in a cleaner way rather > > >>> than spreading this information all over the driver. I'm aware that it > > >>> has grown to that state incrementally, but at this point I think it's > > >>> clear that some abstraction is needed. > > >> You're the maintainer, the decission is of course up to you. > > >> I have generally no problem with reverting patches. If something isn't > > >> ready or plain crap, it should of course be reverted. > > >> But AFAICS, the problem here isn't the patch series itself. It's just > > >> that this series unveils another long standing bug. > > > As I said in my last mail, the important thing is to fix the regression > > > so we don't break any currently working systems. > > > > Johan, I totally agree with you. > > But we have _two_ possibilities to fix this regression. > > 1) revert the whole buncg of patches > > Yes, and this is the way we do it this time. > > I'm afraid that the patches did not get enough review (and I blame > myself for not finding the time to do it sooner) and could still be > improved. No, it's my fault as well, don't blame yourself. I'll go revert all of these for now and send it to Linus for 3.12-final, and then we can start "fresh" in resolving this issue. thanks, greg k-h -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 02:47:27PM +0100, Frank Schäfer wrote: > Am 01.11.2013 14:29, schrieb Johan Hovold: > > On Fri, Nov 01, 2013 at 01:03:24PM +0100, Frank Schäfer wrote: > >> Am 01.11.2013 12:15, schrieb Johan Hovold: > >>> On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: > Am 01.11.2013 11:22, schrieb Johan Hovold: > > I think that adding support for odd baud rates (and the improved device > > detection) can be implemented in a cleaner way and want to have a chance > > to discuss that with you. > Ok, between the lines I can read that you just don't trust this patch > series. ;) > I'm looking forward to your suggestions for a cleaner way to implement > this. :) > >>> You have done a great job here in reverse-engineering the chip and > >>> documenting the quirks. It's not an easy job at all with all the > >>> different chips out there, including pirate ones. > >>> > >>> I just think we should take the safe way here and hold off the changes > >>> another cycle. Get some more people to do more testing (which chip types > >>> do we have among us at the moment?). This would also allow us to come up > >>> with a series which abstracts the differences in a cleaner way rather > >>> than spreading this information all over the driver. I'm aware that it > >>> has grown to that state incrementally, but at this point I think it's > >>> clear that some abstraction is needed. > >> You're the maintainer, the decission is of course up to you. > >> I have generally no problem with reverting patches. If something isn't > >> ready or plain crap, it should of course be reverted. > >> But AFAICS, the problem here isn't the patch series itself. It's just > >> that this series unveils another long standing bug. > > As I said in my last mail, the important thing is to fix the regression > > so we don't break any currently working systems. > > Johan, I totally agree with you. > But we have _two_ possibilities to fix this regression. > 1) revert the whole buncg of patches Yes, and this is the way we do it this time. I'm afraid that the patches did not get enough review (and I blame myself for not finding the time to do it sooner) and could still be improved. > 2) remove the calculation and reproting of the actually set baud rate > with a follow-up patch You call this is a partial revert in a follow up mail, and this could possibly fix the problem we're seeing. However, I think reverting the whole series and fixing the general problem is the way to go at this point in time. This way the other changes will get some more review and testing too, which I think is needed. I'd be happy to help with the HX-device I have and hopefully we can gather enough testers to cover the other types as well. > If we can find out how to fix the workaround for the hardware bug, this > would of course replace 2) and be the best solution, but we are running > out of time. Yes, so this will have to wait a bit. > >> Please also consider that the patch I've sent 5 minutes ago should also > >> needs to be applied to stable. > >> We have the same issue there and it's not restricted to the divisor > >> based baud rate encoding method. > >> The same can happen with the direct encoding method... > > I'm aware of that, but I'm not going to accept a quick hack which might > > or might not fix the problem (you have already submitted three > > "experimental" fixes), and which could cause other problems as well. > The two possibilities are no quick hacks and they are known to work. Your first RFC/RFT was a hack, and apparently didn't even fix the problem according to Mika. I did not look much closer at the other two you marked "experimental". > Concerning the experimental patches: > Yes they are _experimental_ and not yet ready, That's why I do _not_ > want to see them applied. I necer said anything else. > What we're trying to do is to fix the workaround for the hardware bug, > which we have to do in any case. I just wanted to make clear that no fix for the general problem you would come up with today would be enough to to change my mind given that 3.12 likely is to be released on Sunday and that the reverts need to go to Linus today. [ And any fix for the general problem obviously needs to be thought through and properly tested first. ] Sorry, 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 14:47, schrieb Frank Schäfer: > Am 01.11.2013 14:29, schrieb Johan Hovold: >> On Fri, Nov 01, 2013 at 01:03:24PM +0100, Frank Schäfer wrote: >>> Am 01.11.2013 12:15, schrieb Johan Hovold: On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: > Am 01.11.2013 11:22, schrieb Johan Hovold: >> I think that adding support for odd baud rates (and the improved device >> detection) can be implemented in a cleaner way and want to have a chance >> to discuss that with you. > Ok, between the lines I can read that you just don't trust this patch > series. ;) > I'm looking forward to your suggestions for a cleaner way to implement > this. :) You have done a great job here in reverse-engineering the chip and documenting the quirks. It's not an easy job at all with all the different chips out there, including pirate ones. I just think we should take the safe way here and hold off the changes another cycle. Get some more people to do more testing (which chip types do we have among us at the moment?). This would also allow us to come up with a series which abstracts the differences in a cleaner way rather than spreading this information all over the driver. I'm aware that it has grown to that state incrementally, but at this point I think it's clear that some abstraction is needed. >>> You're the maintainer, the decission is of course up to you. >>> I have generally no problem with reverting patches. If something isn't >>> ready or plain crap, it should of course be reverted. >>> But AFAICS, the problem here isn't the patch series itself. It's just >>> that this series unveils another long standing bug. >> As I said in my last mail, the important thing is to fix the regression >> so we don't break any currently working systems. > Johan, I totally agree with you. > But we have _two_ possibilities to fix this regression. > 1) revert the whole buncg of patches > 2) remove the calculation and reproting of the actually set baud rate > with a follow-up patch Just to be clear: 2) isn't yet another change, it's in fact a partial revert. Frank > If we can find out how to fix the workaround for the hardware bug, this > would of course replace 2) and be the best solution, but we are running > out of time. > >>> Please also consider that the patch I've sent 5 minutes ago should also >>> needs to be applied to stable. >>> We have the same issue there and it's not restricted to the divisor >>> based baud rate encoding method. >>> The same can happen with the direct encoding method... >> I'm aware of that, but I'm not going to accept a quick hack which might >> or might not fix the problem (you have already submitted three >> "experimental" fixes), and which could cause other problems as well. > The two possibilities are no quick hacks and they are known to work. > > Concerning the experimental patches: > Yes they are _experimental_ and not yet ready, That's why I do _not_ > want to see them applied. I necer said anything else. > What we're trying to do is to fix the workaround for the hardware bug, > which we have to do in any case. > > Regards, > Frank > >> It's too late in the cycle for this. >> >>> So IMHO, going a step forward and fixing this bug is the better solution. >> Let's revert, and then fix the general baudrate-switching problem during >> 3.13-rc. >> >> After some abstractions are in place and we have a chance to discuss >> which baudrate-setting method should be used when, we can easily add >> back the refined device detection and improved divisor algorithm you >> implemented. >> >> Thanks, >> 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 14:29, schrieb Johan Hovold: > On Fri, Nov 01, 2013 at 01:03:24PM +0100, Frank Schäfer wrote: >> Am 01.11.2013 12:15, schrieb Johan Hovold: >>> On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: Am 01.11.2013 11:22, schrieb Johan Hovold: > I think that adding support for odd baud rates (and the improved device > detection) can be implemented in a cleaner way and want to have a chance > to discuss that with you. Ok, between the lines I can read that you just don't trust this patch series. ;) I'm looking forward to your suggestions for a cleaner way to implement this. :) >>> You have done a great job here in reverse-engineering the chip and >>> documenting the quirks. It's not an easy job at all with all the >>> different chips out there, including pirate ones. >>> >>> I just think we should take the safe way here and hold off the changes >>> another cycle. Get some more people to do more testing (which chip types >>> do we have among us at the moment?). This would also allow us to come up >>> with a series which abstracts the differences in a cleaner way rather >>> than spreading this information all over the driver. I'm aware that it >>> has grown to that state incrementally, but at this point I think it's >>> clear that some abstraction is needed. >> You're the maintainer, the decission is of course up to you. >> I have generally no problem with reverting patches. If something isn't >> ready or plain crap, it should of course be reverted. >> But AFAICS, the problem here isn't the patch series itself. It's just >> that this series unveils another long standing bug. > As I said in my last mail, the important thing is to fix the regression > so we don't break any currently working systems. Johan, I totally agree with you. But we have _two_ possibilities to fix this regression. 1) revert the whole buncg of patches 2) remove the calculation and reproting of the actually set baud rate with a follow-up patch If we can find out how to fix the workaround for the hardware bug, this would of course replace 2) and be the best solution, but we are running out of time. > >> Please also consider that the patch I've sent 5 minutes ago should also >> needs to be applied to stable. >> We have the same issue there and it's not restricted to the divisor >> based baud rate encoding method. >> The same can happen with the direct encoding method... > I'm aware of that, but I'm not going to accept a quick hack which might > or might not fix the problem (you have already submitted three > "experimental" fixes), and which could cause other problems as well. The two possibilities are no quick hacks and they are known to work. Concerning the experimental patches: Yes they are _experimental_ and not yet ready, That's why I do _not_ want to see them applied. I necer said anything else. What we're trying to do is to fix the workaround for the hardware bug, which we have to do in any case. Regards, Frank > It's too late in the cycle for this. > >> So IMHO, going a step forward and fixing this bug is the better solution. > Let's revert, and then fix the general baudrate-switching problem during > 3.13-rc. > > After some abstractions are in place and we have a chance to discuss > which baudrate-setting method should be used when, we can easily add > back the refined device detection and improved divisor algorithm you > implemented. > > Thanks, > 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 01:03:24PM +0100, Frank Schäfer wrote: > Am 01.11.2013 12:15, schrieb Johan Hovold: > > On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: > >> Am 01.11.2013 11:22, schrieb Johan Hovold: > >>> I think that adding support for odd baud rates (and the improved device > >>> detection) can be implemented in a cleaner way and want to have a chance > >>> to discuss that with you. > >> Ok, between the lines I can read that you just don't trust this patch > >> series. ;) > >> I'm looking forward to your suggestions for a cleaner way to implement > >> this. :) > > You have done a great job here in reverse-engineering the chip and > > documenting the quirks. It's not an easy job at all with all the > > different chips out there, including pirate ones. > > > > I just think we should take the safe way here and hold off the changes > > another cycle. Get some more people to do more testing (which chip types > > do we have among us at the moment?). This would also allow us to come up > > with a series which abstracts the differences in a cleaner way rather > > than spreading this information all over the driver. I'm aware that it > > has grown to that state incrementally, but at this point I think it's > > clear that some abstraction is needed. > You're the maintainer, the decission is of course up to you. > I have generally no problem with reverting patches. If something isn't > ready or plain crap, it should of course be reverted. > But AFAICS, the problem here isn't the patch series itself. It's just > that this series unveils another long standing bug. As I said in my last mail, the important thing is to fix the regression so we don't break any currently working systems. > Please also consider that the patch I've sent 5 minutes ago should also > needs to be applied to stable. > We have the same issue there and it's not restricted to the divisor > based baud rate encoding method. > The same can happen with the direct encoding method... I'm aware of that, but I'm not going to accept a quick hack which might or might not fix the problem (you have already submitted three "experimental" fixes), and which could cause other problems as well. It's too late in the cycle for this. > So IMHO, going a step forward and fixing this bug is the better solution. Let's revert, and then fix the general baudrate-switching problem during 3.13-rc. After some abstractions are in place and we have a chance to discuss which baudrate-setting method should be used when, we can easily add back the refined device detection and improved divisor algorithm you implemented. Thanks, 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 12:15, schrieb Johan Hovold: > On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: >> Am 01.11.2013 11:22, schrieb Johan Hovold: > Greg, what do you say about this? Is reverting for 3.12 the correct way > to deal with this, and make sure these fairly invasive patches get some > more testing before being reapplied? > > Reverting would mean reverting a whole bunch of commits though, as they > appear to depend quite heavily on each other: > > 7d26a78 USB: pl2303: distinguish between original and cloned HX chips > 034d152 pl2303: improve the chip type detection/distinction > a77a8c2 pl2303: improve the chip type information output on startup > 73b583a pl2303: simplify the else-if contruct for type_1 chips in > pl2303_startup() > c23bda3 usb: pl2303: add two comments concerning the supported baud rates > with HX chips > 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method > for baud rates < 115200 with HX chips > b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor > based encoding method > e917ba0 usb: pl2303: move the two baud rate encoding methods to separate > functions > b9208c7 usb: pl2303: remove 50 baud from the list of standard baud > rates > 75417d9 usb: pl2303: do not round to the next nearest standard baud rate > for the divisor based baud rate encoding method > 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding > method > b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method > to the "HX" chip type No need to revert all these patches, removing the calculation of the resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() should fix this issue. >>> It could, if the problem I identified above is the only issue here. >> Well, unless you don't find another regression, I see no reason to >> revert. ;) >> >>> However, we have a clear regression and 3.12 is coming out very, very >>> soon. >> Indeed, but fortunately we are also close to a solution/decision. >> The remaining question is, does the HXD work with the improved divisor method or do we have to reintroduce the old method ? >>> This is exactly the kind of issues we can discuss and investigate after >>> reverting the changes. Let's not break any systems meanwhile. >> Well, Mika and you have have such a device, so you can test it. > I'm afraid I can't just drop everything else I'm working with to do that > testing at the moment. No problem. :) In the meantime, Mika has tested it and the improved divisor method works fine. > >> Alternatively, we can apply the patch I've sent that restores the old >> behavior (of course without the calculation of the resulting baud rate). > But that's another change. Late change. [superseeded] > >>> I think that adding support for odd baud rates (and the improved device >>> detection) can be implemented in a cleaner way and want to have a chance >>> to discuss that with you. >> Ok, between the lines I can read that you just don't trust this patch >> series. ;) >> I'm looking forward to your suggestions for a cleaner way to implement >> this. :) > You have done a great job here in reverse-engineering the chip and > documenting the quirks. It's not an easy job at all with all the > different chips out there, including pirate ones. > > I just think we should take the safe way here and hold off the changes > another cycle. Get some more people to do more testing (which chip types > do we have among us at the moment?). This would also allow us to come up > with a series which abstracts the differences in a cleaner way rather > than spreading this information all over the driver. I'm aware that it > has grown to that state incrementally, but at this point I think it's > clear that some abstraction is needed. You're the maintainer, the decission is of course up to you. I have generally no problem with reverting patches. If something isn't ready or plain crap, it should of course be reverted. But AFAICS, the problem here isn't the patch series itself. It's just that this series unveils another long standing bug. Please also consider that the patch I've sent 5 minutes ago should also needs to be applied to stable. We have the same issue there and it's not restricted to the divisor based baud rate encoding method. The same can happen with the direct encoding method... So IMHO, going a step forward and fixing this bug is the better solution. Regards, Frank > I also believe using the direct encoding method also for (known, > supported) baudrates > 115200, as you suggested somewhere, should be > considered (and only fall back to divisor method for other rates if the > chip supports it). > > 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.kerne
Re: [PATCH] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 11:59, schrieb Mika Westerberg: > On Fri, Nov 01, 2013 at 11:45:32AM +0100, Frank Schäfer wrote: >> Am 01.11.2013 11:39, schrieb Mika Westerberg: >>> On Fri, Nov 01, 2013 at 11:06:40AM +0100, Frank Schäfer wrote: No need to revert all these patches, removing the calculation of the resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() should fix this issue. The remaining question is, does the HXD work with the improved divisor method or do we have to reintroduce the old method ? >>> It works, with only removing the baud rate calculation. >> For both baud rates <= 115200 AND > 115200 ? > Yes. Great ! Everything else would have been surprising. I've sent a better patch that should fix this issue 3 minutes ago. Can you test it ? Regards Frank -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 11:44:33AM +0100, Frank Schäfer wrote: > Am 01.11.2013 11:22, schrieb Johan Hovold: > >>> Greg, what do you say about this? Is reverting for 3.12 the correct way > >>> to deal with this, and make sure these fairly invasive patches get some > >>> more testing before being reapplied? > >>> > >>> Reverting would mean reverting a whole bunch of commits though, as they > >>> appear to depend quite heavily on each other: > >>> > >>> 7d26a78 USB: pl2303: distinguish between original and cloned HX chips > >>> 034d152 pl2303: improve the chip type detection/distinction > >>> a77a8c2 pl2303: improve the chip type information output on startup > >>> 73b583a pl2303: simplify the else-if contruct for type_1 chips in > >>> pl2303_startup() > >>> c23bda3 usb: pl2303: add two comments concerning the supported baud rates > >>> with HX chips > >>> 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method > >>> for baud rates < 115200 with HX chips > >>> b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor > >>> based encoding method > >>> e917ba0 usb: pl2303: move the two baud rate encoding methods to separate > >>> functions > >>> b9208c7 usb: pl2303: remove 50 baud from the list of standard baud > >>> rates > >>> 75417d9 usb: pl2303: do not round to the next nearest standard baud rate > >>> for the divisor based baud rate encoding method > >>> 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding > >>> method > >>> b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method > >>> to the "HX" chip type > >> No need to revert all these patches, removing the calculation of the > >> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() > >> should fix this issue. > > It could, if the problem I identified above is the only issue here. > Well, unless you don't find another regression, I see no reason to > revert. ;) > > > However, we have a clear regression and 3.12 is coming out very, very > > soon. > Indeed, but fortunately we are also close to a solution/decision. > > >> The remaining question is, does the HXD work with the improved divisor > >> method or do we have to reintroduce the old method ? > > This is exactly the kind of issues we can discuss and investigate after > > reverting the changes. Let's not break any systems meanwhile. > Well, Mika and you have have such a device, so you can test it. I'm afraid I can't just drop everything else I'm working with to do that testing at the moment. > Alternatively, we can apply the patch I've sent that restores the old > behavior (of course without the calculation of the resulting baud rate). But that's another change. Late change. > > I think that adding support for odd baud rates (and the improved device > > detection) can be implemented in a cleaner way and want to have a chance > > to discuss that with you. > Ok, between the lines I can read that you just don't trust this patch > series. ;) > I'm looking forward to your suggestions for a cleaner way to implement > this. :) You have done a great job here in reverse-engineering the chip and documenting the quirks. It's not an easy job at all with all the different chips out there, including pirate ones. I just think we should take the safe way here and hold off the changes another cycle. Get some more people to do more testing (which chip types do we have among us at the moment?). This would also allow us to come up with a series which abstracts the differences in a cleaner way rather than spreading this information all over the driver. I'm aware that it has grown to that state incrementally, but at this point I think it's clear that some abstraction is needed. I also believe using the direct encoding method also for (known, supported) baudrates > 115200, as you suggested somewhere, should be considered (and only fall back to divisor method for other rates if the chip supports it). 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 11:45:32AM +0100, Frank Schäfer wrote: > Am 01.11.2013 11:39, schrieb Mika Westerberg: > > On Fri, Nov 01, 2013 at 11:06:40AM +0100, Frank Schäfer wrote: > >> No need to revert all these patches, removing the calculation of the > >> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() > >> should fix this issue. > >> The remaining question is, does the HXD work with the improved divisor > >> method or do we have to reintroduce the old method ? > > It works, with only removing the baud rate calculation. > For both baud rates <= 115200 AND > 115200 ? Yes. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 11:39, schrieb Mika Westerberg: > On Fri, Nov 01, 2013 at 11:06:40AM +0100, Frank Schäfer wrote: >> No need to revert all these patches, removing the calculation of the >> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() >> should fix this issue. >> The remaining question is, does the HXD work with the improved divisor >> method or do we have to reintroduce the old method ? > It works, with only removing the baud rate calculation. For both baud rates <= 115200 AND > 115200 ? Frank -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 11:22, schrieb Johan Hovold: >>> Greg, what do you say about this? Is reverting for 3.12 the correct way >>> to deal with this, and make sure these fairly invasive patches get some >>> more testing before being reapplied? >>> >>> Reverting would mean reverting a whole bunch of commits though, as they >>> appear to depend quite heavily on each other: >>> >>> 7d26a78 USB: pl2303: distinguish between original and cloned HX chips >>> 034d152 pl2303: improve the chip type detection/distinction >>> a77a8c2 pl2303: improve the chip type information output on startup >>> 73b583a pl2303: simplify the else-if contruct for type_1 chips in >>> pl2303_startup() >>> c23bda3 usb: pl2303: add two comments concerning the supported baud rates >>> with HX chips >>> 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method >>> for baud rates < 115200 with HX chips >>> b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor >>> based encoding method >>> e917ba0 usb: pl2303: move the two baud rate encoding methods to separate >>> functions >>> b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates >>> 75417d9 usb: pl2303: do not round to the next nearest standard baud rate >>> for the divisor based baud rate encoding method >>> 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method >>> b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method >>> to the "HX" chip type >> No need to revert all these patches, removing the calculation of the >> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() >> should fix this issue. > It could, if the problem I identified above is the only issue here. Well, unless you don't find another regression, I see no reason to revert. ;) > However, we have a clear regression and 3.12 is coming out very, very > soon. Indeed, but fortunately we are also close to a solution/decision. > >> The remaining question is, does the HXD work with the improved divisor >> method or do we have to reintroduce the old method ? > This is exactly the kind of issues we can discuss and investigate after > reverting the changes. Let's not break any systems meanwhile. Well, Mika and you have have such a device, so you can test it. Alternatively, we can apply the patch I've sent that restores the old behavior (of course without the calculation of the resulting baud rate). > I think that adding support for odd baud rates (and the improved device > detection) can be implemented in a cleaner way and want to have a chance > to discuss that with you. Ok, between the lines I can read that you just don't trust this patch series. ;) I'm looking forward to your suggestions for a cleaner way to implement this. :) Regards, Frank > > Thanks, > 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 11:06:40AM +0100, Frank Schäfer wrote: > No need to revert all these patches, removing the calculation of the > resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor() > should fix this issue. > The remaining question is, does the HXD work with the improved divisor > method or do we have to reintroduce the old method ? It works, with only removing the baud rate calculation. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Fri, Nov 01, 2013 at 11:06:40AM +0100, Frank Schäfer wrote:
> Am 31.10.2013 21:56, schrieb Johan Hovold:
> > On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
> >> On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
> >>> Am 31.10.2013 13:30, schrieb Mika Westerberg:
> On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
> > 2) comment out the following line at the end of
> > pl2303_baudrate_encode_divisor_HXD():
> >
> > baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
> This seems to fix the problem :)
>
> Once the line is commented out, the serial console works fine with the
> speeds I've been testing (115200, 230400 and 460800).
> >>> Urgh... so it seems getty gets confused if the actually set baud rate is
> >>> reported back.
> >>> The kernel still reports the standard baud rate (e.g. B230400), it only
> >>> sets the exact value in the c_ispeed and c_ospeed fields of the termios
> >>> struct.
> >>> So even if getty doesn't support non-standard baud rates, everything
> >>> should be fine.
> >>> I'll have to take a closer look at this issue later.
> > I think I know what's going on. The pl2303 is known to loose bytes when
> > changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
> > switching regression")).
> >
> > Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
> > rate encoding method") introduced a regression when it started reporting
> > back the baudrates determined by the divisor algorithm.
> >
> > Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
> > will always return true -- even when userspace isn't requesting a
> > different baudrate.
> Hmm... so there is a bug in tty_termios_hw_change() ?
Not necessarily.
> >>> Ok, so now let's see if the fixed/improved divisor based method also
> >>> works for the HXD chip if we don't report the actually set baud rate.
> >>> Try commenting out the line
> >>>
> >>> baud = 1200 * 32 / ((1 << A) * B);
> >>>
> >>> at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel.
> >>> It's unlikely that this makes baud rates < 115200 working, but testing
> >>> both cases doesn't harm. ;)
> > I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
> > I've verified that not returning the calculated baudrate makes this
> > particular issue *seem* to go away.
> >
> > This obviously has to be fixed or reverted quickly.
> Indeed.
>
> > Greg, what do you say about this? Is reverting for 3.12 the correct way
> > to deal with this, and make sure these fairly invasive patches get some
> > more testing before being reapplied?
> >
> > Reverting would mean reverting a whole bunch of commits though, as they
> > appear to depend quite heavily on each other:
> >
> > 7d26a78 USB: pl2303: distinguish between original and cloned HX chips
> > 034d152 pl2303: improve the chip type detection/distinction
> > a77a8c2 pl2303: improve the chip type information output on startup
> > 73b583a pl2303: simplify the else-if contruct for type_1 chips in
> > pl2303_startup()
> > c23bda3 usb: pl2303: add two comments concerning the supported baud rates
> > with HX chips
> > 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method
> > for baud rates < 115200 with HX chips
> > b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor
> > based encoding method
> > e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
> > functions
> > b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
> > 75417d9 usb: pl2303: do not round to the next nearest standard baud rate
> > for the divisor based baud rate encoding method
> > 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
> > b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method
> > to the "HX" chip type
> No need to revert all these patches, removing the calculation of the
> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor()
> should fix this issue.
It could, if the problem I identified above is the only issue here.
However, we have a clear regression and 3.12 is coming out very, very
soon.
> The remaining question is, does the HXD work with the improved divisor
> method or do we have to reintroduce the old method ?
This is exactly the kind of issues we can discuss and investigate after
reverting the changes. Let's not break any systems meanwhile.
I think that adding support for odd baud rates (and the improved device
detection) can be implemented in a cleaner way and want to have a chance
to discuss that with you.
Thanks,
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 01.11.2013 11:06, schrieb Frank Schäfer:
> Am 31.10.2013 21:56, schrieb Johan Hovold:
>> On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
>>> On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
Am 31.10.2013 13:30, schrieb Mika Westerberg:
> On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
>> 2) comment out the following line at the end of
>> pl2303_baudrate_encode_divisor_HXD():
>>
>> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
> This seems to fix the problem :)
>
> Once the line is commented out, the serial console works fine with the
> speeds I've been testing (115200, 230400 and 460800).
Urgh... so it seems getty gets confused if the actually set baud rate is
reported back.
The kernel still reports the standard baud rate (e.g. B230400), it only
sets the exact value in the c_ispeed and c_ospeed fields of the termios
struct.
So even if getty doesn't support non-standard baud rates, everything
should be fine.
I'll have to take a closer look at this issue later.
>> I think I know what's going on. The pl2303 is known to loose bytes when
>> changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
>> switching regression")).
>>
>> Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
>> rate encoding method") introduced a regression when it started reporting
>> back the baudrates determined by the divisor algorithm.
>>
>> Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
>> will always return true -- even when userspace isn't requesting a
>> different baudrate.
> Hmm... so there is a bug in tty_termios_hw_change() ?
Ahh... no... I see what's going on.
I assume we have to compare the resulting encoded baud rates to check if
the settings changed.
But that's something for the next kernel...
Frank
>
Ok, so now let's see if the fixed/improved divisor based method also
works for the HXD chip if we don't report the actually set baud rate.
Try commenting out the line
baud = 1200 * 32 / ((1 << A) * B);
at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel.
It's unlikely that this makes baud rates < 115200 working, but testing
both cases doesn't harm. ;)
>> I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
>> I've verified that not returning the calculated baudrate makes this
>> particular issue *seem* to go away.
>>
>> This obviously has to be fixed or reverted quickly.
> Indeed.
>
>> Greg, what do you say about this? Is reverting for 3.12 the correct way
>> to deal with this, and make sure these fairly invasive patches get some
>> more testing before being reapplied?
>>
>> Reverting would mean reverting a whole bunch of commits though, as they
>> appear to depend quite heavily on each other:
>>
>> 7d26a78 USB: pl2303: distinguish between original and cloned HX chips
>> 034d152 pl2303: improve the chip type detection/distinction
>> a77a8c2 pl2303: improve the chip type information output on startup
>> 73b583a pl2303: simplify the else-if contruct for type_1 chips in
>> pl2303_startup()
>> c23bda3 usb: pl2303: add two comments concerning the supported baud rates
>> with HX chips
>> 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method
>> for baud rates < 115200 with HX chips
>> b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor
>> based encoding method
>> e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
>> functions
>> b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
>> 75417d9 usb: pl2303: do not round to the next nearest standard baud rate for
>> the divisor based baud rate encoding method
>> 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
>> b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method to
>> the "HX" chip type
> No need to revert all these patches, removing the calculation of the
> resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor()
> should fix this issue.
> The remaining question is, does the HXD work with the improved divisor
> method or do we have to reintroduce the old method ?
>
> Regards,
> Frank
>
>> 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 31.10.2013 21:56, schrieb Johan Hovold:
> On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
>> On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
>>> Am 31.10.2013 13:30, schrieb Mika Westerberg:
On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
> 2) comment out the following line at the end of
> pl2303_baudrate_encode_divisor_HXD():
>
> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
This seems to fix the problem :)
Once the line is commented out, the serial console works fine with the
speeds I've been testing (115200, 230400 and 460800).
>>> Urgh... so it seems getty gets confused if the actually set baud rate is
>>> reported back.
>>> The kernel still reports the standard baud rate (e.g. B230400), it only
>>> sets the exact value in the c_ispeed and c_ospeed fields of the termios
>>> struct.
>>> So even if getty doesn't support non-standard baud rates, everything
>>> should be fine.
>>> I'll have to take a closer look at this issue later.
> I think I know what's going on. The pl2303 is known to loose bytes when
> changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
> switching regression")).
>
> Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
> rate encoding method") introduced a regression when it started reporting
> back the baudrates determined by the divisor algorithm.
>
> Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
> will always return true -- even when userspace isn't requesting a
> different baudrate.
Hmm... so there is a bug in tty_termios_hw_change() ?
>>> Ok, so now let's see if the fixed/improved divisor based method also
>>> works for the HXD chip if we don't report the actually set baud rate.
>>> Try commenting out the line
>>>
>>> baud = 1200 * 32 / ((1 << A) * B);
>>>
>>> at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel.
>>> It's unlikely that this makes baud rates < 115200 working, but testing
>>> both cases doesn't harm. ;)
> I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
> I've verified that not returning the calculated baudrate makes this
> particular issue *seem* to go away.
>
> This obviously has to be fixed or reverted quickly.
Indeed.
> Greg, what do you say about this? Is reverting for 3.12 the correct way
> to deal with this, and make sure these fairly invasive patches get some
> more testing before being reapplied?
>
> Reverting would mean reverting a whole bunch of commits though, as they
> appear to depend quite heavily on each other:
>
> 7d26a78 USB: pl2303: distinguish between original and cloned HX chips
> 034d152 pl2303: improve the chip type detection/distinction
> a77a8c2 pl2303: improve the chip type information output on startup
> 73b583a pl2303: simplify the else-if contruct for type_1 chips in
> pl2303_startup()
> c23bda3 usb: pl2303: add two comments concerning the supported baud rates
> with HX chips
> 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method for
> baud rates < 115200 with HX chips
> b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor
> based encoding method
> e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
> functions
> b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
> 75417d9 usb: pl2303: do not round to the next nearest standard baud rate for
> the divisor based baud rate encoding method
> 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
> b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method to
> the "HX" chip type
No need to revert all these patches, removing the calculation of the
resulting baud rateat the end of fcn pl2303_baudrate_encode_divisor()
should fix this issue.
The remaining question is, does the HXD work with the improved divisor
method or do we have to reintroduce the old method ?
Regards,
Frank
> 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Thu, Oct 31, 2013 at 02:03:22PM -0700, Greg KH wrote:
> On Thu, Oct 31, 2013 at 09:56:25PM +0100, Johan Hovold wrote:
> > On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
> > > On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
> > > > Am 31.10.2013 13:30, schrieb Mika Westerberg:
> > > >> On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
> > > >>> 2) comment out the following line at the end of
> > > >>> pl2303_baudrate_encode_divisor_HXD():
> > > >>>
> > > >>> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
> > > >> This seems to fix the problem :)
> > > >>
> > > >> Once the line is commented out, the serial console works fine with the
> > > >> speeds I've been testing (115200, 230400 and 460800).
> > > >
> > > > Urgh... so it seems getty gets confused if the actually set baud rate is
> > > > reported back.
> > > > The kernel still reports the standard baud rate (e.g. B230400), it only
> > > > sets the exact value in the c_ispeed and c_ospeed fields of the termios
> > > > struct.
> > > > So even if getty doesn't support non-standard baud rates, everything
> > > > should be fine.
> > > > I'll have to take a closer look at this issue later.
> >
> > I think I know what's going on. The pl2303 is known to loose bytes when
> > changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
> > switching regression")).
> >
> > Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
> > rate encoding method") introduced a regression when it started reporting
> > back the baudrates determined by the divisor algorithm.
> >
> > Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
> > will always return true -- even when userspace isn't requesting a
> > different baudrate.
> >
> > > > Ok, so now let's see if the fixed/improved divisor based method also
> > > > works for the HXD chip if we don't report the actually set baud rate.
> > > > Try commenting out the line
> > > >
> > > > baud = 1200 * 32 / ((1 << A) * B);
> > > >
> > > > at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc
> > > > kernel.
> > > > It's unlikely that this makes baud rates < 115200 working, but testing
> > > > both cases doesn't harm. ;)
> >
> > I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
> > I've verified that not returning the calculated baudrate makes this
> > particular issue *seem* to go away.
> >
> > This obviously has to be fixed or reverted quickly.
> >
> > Greg, what do you say about this? Is reverting for 3.12 the correct way
> > to deal with this, and make sure these fairly invasive patches get some
> > more testing before being reapplied?
> >
> > Reverting would mean reverting a whole bunch of commits though, as they
> > appear to depend quite heavily on each other:
>
> Ick :(
>
> Would fixing the "return the baudrate" issue solve this in an easier way
> than to revert all of these? That might be an easier, and smaller,
> patch that can get added to 3.12-stable, right?
It would possibly be sufficient, but I don't have much time the next
couple of days to do much further digging and verification of this.
There could be other ways to trigger the problem and I'd like to get a
chance to look into that as well.
> > 7d26a78 USB: pl2303: distinguish between original and cloned HX chips
> > 034d152 pl2303: improve the chip type detection/distinction
> > a77a8c2 pl2303: improve the chip type information output on startup
> > 73b583a pl2303: simplify the else-if contruct for type_1 chips in
> > pl2303_startup()
> > c23bda3 usb: pl2303: add two comments concerning the supported baud rates
> > with HX chips
> > 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method
> > for baud rates < 115200 with HX chips
> > b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor
> > based encoding method
> > e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
> > functions
> > b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
> > 75417d9 usb: pl2303: do not round to the next nearest standard baud rate
> > for the divisor based baud rate encoding method
> > 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
> > b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method
> > to the "HX" chip type
>
> Or I can just revert all of them for 3.12-stable as well, let me know
> which one you want me to do.
Yes, I think we should revert (if possible before 3.12 is out), clean up
the above series and do some more verification before reapplying as a
single, clean series (there appears to already be a regression with fix
within the patches listed above).
Thanks,
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Thu, Oct 31, 2013 at 09:56:25PM +0100, Johan Hovold wrote:
> On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
> > On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
> > > Am 31.10.2013 13:30, schrieb Mika Westerberg:
> > >> On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
> > >>> 2) comment out the following line at the end of
> > >>> pl2303_baudrate_encode_divisor_HXD():
> > >>>
> > >>> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
> > >> This seems to fix the problem :)
> > >>
> > >> Once the line is commented out, the serial console works fine with the
> > >> speeds I've been testing (115200, 230400 and 460800).
> > >
> > > Urgh... so it seems getty gets confused if the actually set baud rate is
> > > reported back.
> > > The kernel still reports the standard baud rate (e.g. B230400), it only
> > > sets the exact value in the c_ispeed and c_ospeed fields of the termios
> > > struct.
> > > So even if getty doesn't support non-standard baud rates, everything
> > > should be fine.
> > > I'll have to take a closer look at this issue later.
>
> I think I know what's going on. The pl2303 is known to loose bytes when
> changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
> switching regression")).
>
> Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
> rate encoding method") introduced a regression when it started reporting
> back the baudrates determined by the divisor algorithm.
>
> Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
> will always return true -- even when userspace isn't requesting a
> different baudrate.
>
> > > Ok, so now let's see if the fixed/improved divisor based method also
> > > works for the HXD chip if we don't report the actually set baud rate.
> > > Try commenting out the line
> > >
> > > baud = 1200 * 32 / ((1 << A) * B);
> > >
> > > at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel.
> > > It's unlikely that this makes baud rates < 115200 working, but testing
> > > both cases doesn't harm. ;)
>
> I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
> I've verified that not returning the calculated baudrate makes this
> particular issue *seem* to go away.
>
> This obviously has to be fixed or reverted quickly.
>
> Greg, what do you say about this? Is reverting for 3.12 the correct way
> to deal with this, and make sure these fairly invasive patches get some
> more testing before being reapplied?
>
> Reverting would mean reverting a whole bunch of commits though, as they
> appear to depend quite heavily on each other:
Ick :(
Would fixing the "return the baudrate" issue solve this in an easier way
than to revert all of these? That might be an easier, and smaller,
patch that can get added to 3.12-stable, right?
> 7d26a78 USB: pl2303: distinguish between original and cloned HX chips
> 034d152 pl2303: improve the chip type detection/distinction
> a77a8c2 pl2303: improve the chip type information output on startup
> 73b583a pl2303: simplify the else-if contruct for type_1 chips in
> pl2303_startup()
> c23bda3 usb: pl2303: add two comments concerning the supported baud rates
> with HX chips
> 61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method for
> baud rates < 115200 with HX chips
> b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor
> based encoding method
> e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
> functions
> b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
> 75417d9 usb: pl2303: do not round to the next nearest standard baud rate for
> the divisor based baud rate encoding method
> 57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
> b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method to
> the "HX" chip type
Or I can just revert all of them for 3.12-stable as well, let me know
which one you want me to do.
thanks,
greg k-h
--
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Thu, Oct 31, 2013 at 09:26:06PM +0100, Johan Hovold wrote:
> On Thu, Oct 31, 2013 at 7:45 PM, Frank Schäfer wrote:
> > Am 31.10.2013 13:30, schrieb Mika Westerberg:
> >> On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote:
> >>> 2) comment out the following line at the end of
> >>> pl2303_baudrate_encode_divisor_HXD():
> >>>
> >>> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
> >> This seems to fix the problem :)
> >>
> >> Once the line is commented out, the serial console works fine with the
> >> speeds I've been testing (115200, 230400 and 460800).
> >
> > Urgh... so it seems getty gets confused if the actually set baud rate is
> > reported back.
> > The kernel still reports the standard baud rate (e.g. B230400), it only
> > sets the exact value in the c_ispeed and c_ospeed fields of the termios
> > struct.
> > So even if getty doesn't support non-standard baud rates, everything
> > should be fine.
> > I'll have to take a closer look at this issue later.
I think I know what's going on. The pl2303 is known to loose bytes when
changing the termios settings (see commit bf5e5834b ("pl2303: Fix mode
switching regression")).
Your commit 57ce61aad ("usb: pl2303: fix+improve the divisor based baud
rate encoding method") introduced a regression when it started reporting
back the baudrates determined by the divisor algorithm.
Since that commit, tty_termios_hw_change(&tty->termios, old_termios)
will always return true -- even when userspace isn't requesting a
different baudrate.
> > Ok, so now let's see if the fixed/improved divisor based method also
> > works for the HXD chip if we don't report the actually set baud rate.
> > Try commenting out the line
> >
> > baud = 1200 * 32 / ((1 << A) * B);
> >
> > at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel.
> > It's unlikely that this makes baud rates < 115200 working, but testing
> > both cases doesn't harm. ;)
I can trigger the problem here with my HXD/EA/RA/SA-device as well, and
I've verified that not returning the calculated baudrate makes this
particular issue *seem* to go away.
This obviously has to be fixed or reverted quickly.
Greg, what do you say about this? Is reverting for 3.12 the correct way
to deal with this, and make sure these fairly invasive patches get some
more testing before being reapplied?
Reverting would mean reverting a whole bunch of commits though, as they
appear to depend quite heavily on each other:
7d26a78 USB: pl2303: distinguish between original and cloned HX chips
034d152 pl2303: improve the chip type detection/distinction
a77a8c2 pl2303: improve the chip type information output on startup
73b583a pl2303: simplify the else-if contruct for type_1 chips in
pl2303_startup()
c23bda3 usb: pl2303: add two comments concerning the supported baud rates with
HX chips
61fa8d6 usb: pl2303: also use the divisor based baud rate encoding method for
baud rates < 115200 with HX chips
b5c16c6 usb: pl2303: increase the allowed baud rate range for the divisor based
encoding method
e917ba0 usb: pl2303: move the two baud rate encoding methods to separate
functions
b9208c7 usb: pl2303: remove 50 baud from the list of standard baud rates
75417d9 usb: pl2303: do not round to the next nearest standard baud rate for
the divisor based baud rate encoding method
57ce61a usb: pl2303: fix+improve the divsor based baud rate encoding method
b8bdad6 USB: pl2303: restrict the divisor based baud rate encoding method to
the "HX" chip type
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 31.10.2013 13:30, schrieb Mika Westerberg: > On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote: >> 2) comment out the following line at the end of >> pl2303_baudrate_encode_divisor_HXD(): >> >> baud = 1200 * 32 / ((1 << buf[1]) * buf[0]); > This seems to fix the problem :) > > Once the line is commented out, the serial console works fine with the > speeds I've been testing (115200, 230400 and 460800). Urgh... so it seems getty gets confused if the actually set baud rate is reported back. The kernel still reports the standard baud rate (e.g. B230400), it only sets the exact value in the c_ispeed and c_ospeed fields of the termios struct. So even if getty doesn't support non-standard baud rates, everything should be fine. I'll have to take a closer look at this issue later. Ok, so now let's see if the fixed/improved divisor based method also works for the HXD chip if we don't report the actually set baud rate. Try commenting out the line baud = 1200 * 32 / ((1 << A) * B); at the end of pl2303_baudrate_encode_divisor() of a clean 3.12-rc kernel. It's unlikely that this makes baud rates < 115200 working, but testing both cases doesn't harm. ;) Regards, Frank -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Thu, Oct 31, 2013 at 01:02:56PM +0100, Frank Schäfer wrote: > 2) comment out the following line at the end of > pl2303_baudrate_encode_divisor_HXD(): > > baud = 1200 * 32 / ((1 << buf[1]) * buf[0]); This seems to fix the problem :) Once the line is commented out, the serial console works fine with the speeds I've been testing (115200, 230400 and 460800). -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 31.10.2013 13:03, schrieb Mika Westerberg:
> On Thu, Oct 31, 2013 at 01:09:55PM +0200, Mika Westerberg wrote:
>> On Wed, Oct 30, 2013 at 11:14:34PM +0100, Frank Schäfer wrote:
>>> Hmm... try to replace the whole pl2303.c from 3.12 with the one from 3.11.
>>> If it makes no difference, it's not a pl2303 issue.
>> I did that and the 3.11 pl2303.c works (whereas 3.12 doesn't).
>>
>> Can you tell me why do we even want to use this "divisor" based calculation
>> if we can do the same with direct method?
>>
>> I mean why the driver can't do this:
>>
>> 1) Try direct method for *all* chips.
>> 2) If it succeeds, use that value. Then we don't get any difference
>> between actual and set baud rate.
>> 3) If it fails, then and only then use "divisor" based method.
>>
>> I would expect that the above cures my problem and possibly others who
>> might have affected by this regression.
> Something like the patch below.
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index bedf8e47713b..85f3fa4afc81 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -461,11 +461,11 @@ static void pl2303_encode_baudrate(struct tty_struct
> *tty,
> enum pl2303_type type,
> u8 buf[4])
> {
> - int baud;
> + int baud_req, baud;
>
> - baud = tty_get_baud_rate(tty);
> - dev_dbg(&port->dev, "baud requested = %d\n", baud);
> - if (!baud)
> + baud_req = tty_get_baud_rate(tty);
> + dev_dbg(&port->dev, "baud requested = %d\n", baud_req);
> + if (!baud_req)
> return;
> /*
>* There are two methods for setting/encoding the baud rate
> @@ -480,10 +480,10 @@ static void pl2303_encode_baudrate(struct tty_struct
> *tty,
>* the device likely uses the same baud rate generator for both methods
>* so that there is likley no difference.
>*/
> - if (type == type_0 || type == type_1 || type == HX_CLONE)
> - baud = pl2303_baudrate_encode_direct(baud, type, buf);
> - else
> - baud = pl2303_baudrate_encode_divisor(baud, type, buf);
> + baud = pl2303_baudrate_encode_direct(baud_req, type, buf);
> + if (baud != baud_req)
> + baud = pl2303_baudrate_encode_divisor(baud_req, type, buf);
> +
That doesn't work.
The direct encoding always succeeds.
But if an unsupported baud rate is set, original chips silently run at
9600 baud and the Chinese clones produce data corruption. :/
Frank
> /* Save resulting baud rate */
> tty_encode_baud_rate(tty, baud, baud);
> dev_dbg(&port->dev, "baud set = %d\n", baud);
--
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 31.10.2013 12:09, schrieb Mika Westerberg: > On Wed, Oct 30, 2013 at 11:14:34PM +0100, Frank Schäfer wrote: >> Hmm... try to replace the whole pl2303.c from 3.12 with the one from 3.11. >> If it makes no difference, it's not a pl2303 issue. > I did that and the 3.11 pl2303.c works (whereas 3.12 doesn't). Ok, here a two more things to test (on top of 3.12-rc + patch): 1) move the line port->port.drain_delay = 256; from pl2303_port_probe() back to the end pl2303_open(). 2) comment out the following line at the end of pl2303_baudrate_encode_divisor_HXD(): baud = 1200 * 32 / ((1 << buf[1]) * buf[0]); 3) Verify that the chip is detected as HXD_EA_RA_SA (in debug mode, there should be a corresponding dmesg line). These are the only remaining differences between 3.11 and 3.12. > > Can you tell me why do we even want to use this "divisor" based calculation > if we can do the same with direct method? It seems that some of the newer chips don't work with the direct method at baud rates > 115200. See commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above 115200"). As a said, the commit description is awful and the author doesn't reply to emails. But Prolific states that newer chips need a different baud rate handling, too, which indicates that this commit makes sense. So if we revert this old commit, we likely cause a regression with other pl2303 chips. Apart from that, the divisor based baud rate encoding has been working well since kernel 3.1. The divisor based baud rate method also allows (nearly) continious baud rate adjustment, while the direct method only supports a fixed set of standrad baud rates. Regards, Frank > > I mean why the driver can't do this: > > 1) Try direct method for *all* chips. > 2) If it succeeds, use that value. Then we don't get any difference > between actual and set baud rate. > 3) If it fails, then and only then use "divisor" based method. > > I would expect that the above cures my problem and possibly others who > might have affected by this regression. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Thu, Oct 31, 2013 at 01:09:55PM +0200, Mika Westerberg wrote:
> On Wed, Oct 30, 2013 at 11:14:34PM +0100, Frank Schäfer wrote:
> > Hmm... try to replace the whole pl2303.c from 3.12 with the one from 3.11.
> > If it makes no difference, it's not a pl2303 issue.
>
> I did that and the 3.11 pl2303.c works (whereas 3.12 doesn't).
>
> Can you tell me why do we even want to use this "divisor" based calculation
> if we can do the same with direct method?
>
> I mean why the driver can't do this:
>
> 1) Try direct method for *all* chips.
> 2) If it succeeds, use that value. Then we don't get any difference
> between actual and set baud rate.
> 3) If it fails, then and only then use "divisor" based method.
>
> I would expect that the above cures my problem and possibly others who
> might have affected by this regression.
Something like the patch below.
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e47713b..85f3fa4afc81 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -461,11 +461,11 @@ static void pl2303_encode_baudrate(struct tty_struct *tty,
enum pl2303_type type,
u8 buf[4])
{
- int baud;
+ int baud_req, baud;
- baud = tty_get_baud_rate(tty);
- dev_dbg(&port->dev, "baud requested = %d\n", baud);
- if (!baud)
+ baud_req = tty_get_baud_rate(tty);
+ dev_dbg(&port->dev, "baud requested = %d\n", baud_req);
+ if (!baud_req)
return;
/*
* There are two methods for setting/encoding the baud rate
@@ -480,10 +480,10 @@ static void pl2303_encode_baudrate(struct tty_struct *tty,
* the device likely uses the same baud rate generator for both methods
* so that there is likley no difference.
*/
- if (type == type_0 || type == type_1 || type == HX_CLONE)
- baud = pl2303_baudrate_encode_direct(baud, type, buf);
- else
- baud = pl2303_baudrate_encode_divisor(baud, type, buf);
+ baud = pl2303_baudrate_encode_direct(baud_req, type, buf);
+ if (baud != baud_req)
+ baud = pl2303_baudrate_encode_divisor(baud_req, type, buf);
+
/* Save resulting baud rate */
tty_encode_baud_rate(tty, baud, baud);
dev_dbg(&port->dev, "baud set = %d\n", baud);
--
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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Wed, Oct 30, 2013 at 11:14:34PM +0100, Frank Schäfer wrote: > Hmm... try to replace the whole pl2303.c from 3.12 with the one from 3.11. > If it makes no difference, it's not a pl2303 issue. I did that and the 3.11 pl2303.c works (whereas 3.12 doesn't). Can you tell me why do we even want to use this "divisor" based calculation if we can do the same with direct method? I mean why the driver can't do this: 1) Try direct method for *all* chips. 2) If it succeeds, use that value. Then we don't get any difference between actual and set baud rate. 3) If it fails, then and only then use "divisor" based method. I would expect that the above cures my problem and possibly others who might have affected by this regression. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 30.10.2013 23:07, schrieb Frank Schäfer: > Am 30.10.2013 09:45, schrieb Mika Westerberg: >> On Tue, Oct 29, 2013 at 06:42:49PM +0100, Frank Schäfer wrote: >>> Am 29.10.2013 18:12, schrieb Frank Schäfer: Am 29.10.2013 10:07, schrieb Mika Westerberg: > On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: >> Mika Westerberg has reported that the fixed+improved divisor based baud >> rate >> encoding method doesn't work anymore with his HXD device. >> So until we've found out what's going on, reintroduce the old encoding >> algorithm >> and use it for this and all newer chips for baud rates > 115200 baud. >> Also switch back to the direct encoding method for baud rates <= 115200, >> because >> it's unclear if the old divisor based encoding algorithm works for them. >> >> Reported-by: Mika Westerberg >> Signed-off-by: Frank Schäfer > Tried this and with 115200 it works and fixes the problem. However, with > speeds like 230400 and 460800 it still corrupts data. Well, with this patch we go back to what we did since kernel 3.1 (since commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above 115200"), so you should face the same problems with these kernels, too. >>> I've double checked that. With this patch applied to 3.12-rc the baud >>> rate encoding is exactly the same as in 3.11 (for HXD chips). >>> >>> Are you sure your test setup is reliable / comparable ? Could you please >>> re-check your results ? >> I re-tested with 3.11 and 3.12-rc7 with this patch applied. With 3.11 all >> tried speeds (115200, 230400 and 460800) work fine. With 3.12-rc7 + this >> patch, 115200 works but 230400 and 460800 corrupt data. >> >> My test setup is such that I just start getty on ttyUSB0 with given speed >> and then connect to it using picocom. Once logged in, command like 'ls' >> will return listing that is corrupted somehow: >> >> [root@tbt02 /]# ls >> NH$�j�I[0m/ init*linuxrc@ opt/ run@ tmp/r/ >> etc/ lib/ media/ proc/sbin/usr/ >> >> It is supposed to look like: >> >> [root@tbt02 /]# ls >> bin/ home/lib32@ mnt/ root/sys/ var/ >> dev/ init*linuxrc@ opt/ run@ tmp/ >> etc/ lib/ media/ proc/sbin/usr/ > I tried to reproduce your problems (with a HX device) and got some data > corruption, too. > It seem that I have to wait ~30-60 seconds after the device(s) have been > plugged in before the devices can be used. > During this time, I can see the Rx/Tx LED of my FTDI device (which I'm > using as communication partner) blinking. > If I start getty + picocom earlier, I get data corruption. If I wait a > minute, everthing works perfectly. > But in both cases, there is no difference between kernel 3.11 and 3.12. > > At the moment, I'm not sure what to do. If your problems (with baud > rates > 115200) are really a 3.11-> 3.12 regression, than going back to > the old baud rate encoding should fix it. > If it doesn't, then the conclusion would be that the problem must be > something else. > Let me sleep a night on this... > > Regards, > Frank Schäfer Hmm... try to replace the whole pl2303.c from 3.12 with the one from 3.11. If it makes no difference, it's not a pl2303 issue. Frank -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 30.10.2013 09:45, schrieb Mika Westerberg: > On Tue, Oct 29, 2013 at 06:42:49PM +0100, Frank Schäfer wrote: >> Am 29.10.2013 18:12, schrieb Frank Schäfer: >>> Am 29.10.2013 10:07, schrieb Mika Westerberg: On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: > Mika Westerberg has reported that the fixed+improved divisor based baud > rate > encoding method doesn't work anymore with his HXD device. > So until we've found out what's going on, reintroduce the old encoding > algorithm > and use it for this and all newer chips for baud rates > 115200 baud. > Also switch back to the direct encoding method for baud rates <= 115200, > because > it's unclear if the old divisor based encoding algorithm works for them. > > Reported-by: Mika Westerberg > Signed-off-by: Frank Schäfer Tried this and with 115200 it works and fixes the problem. However, with speeds like 230400 and 460800 it still corrupts data. >>> Well, with this patch we go back to what we did since kernel 3.1 (since >>> commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above >>> 115200"), so you should face the same problems with these kernels, too. >> I've double checked that. With this patch applied to 3.12-rc the baud >> rate encoding is exactly the same as in 3.11 (for HXD chips). >> >> Are you sure your test setup is reliable / comparable ? Could you please >> re-check your results ? > I re-tested with 3.11 and 3.12-rc7 with this patch applied. With 3.11 all > tried speeds (115200, 230400 and 460800) work fine. With 3.12-rc7 + this > patch, 115200 works but 230400 and 460800 corrupt data. > > My test setup is such that I just start getty on ttyUSB0 with given speed > and then connect to it using picocom. Once logged in, command like 'ls' > will return listing that is corrupted somehow: > > [root@tbt02 /]# ls > NH$�j�I[0m/ init*linuxrc@ opt/ run@ tmp/r/ > etc/ lib/ media/ proc/sbin/usr/ > > It is supposed to look like: > > [root@tbt02 /]# ls > bin/ home/lib32@ mnt/ root/sys/ var/ > dev/ init*linuxrc@ opt/ run@ tmp/ > etc/ lib/ media/ proc/sbin/usr/ I tried to reproduce your problems (with a HX device) and got some data corruption, too. It seem that I have to wait ~30-60 seconds after the device(s) have been plugged in before the devices can be used. During this time, I can see the Rx/Tx LED of my FTDI device (which I'm using as communication partner) blinking. If I start getty + picocom earlier, I get data corruption. If I wait a minute, everthing works perfectly. But in both cases, there is no difference between kernel 3.11 and 3.12. At the moment, I'm not sure what to do. If your problems (with baud rates > 115200) are really a 3.11-> 3.12 regression, than going back to the old baud rate encoding should fix it. If it doesn't, then the conclusion would be that the problem must be something else. Let me sleep a night on this... Regards, Frank Schäfer -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Tue, Oct 29, 2013 at 06:42:49PM +0100, Frank Schäfer wrote: > Am 29.10.2013 18:12, schrieb Frank Schäfer: > > Am 29.10.2013 10:07, schrieb Mika Westerberg: > >> On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: > >>> Mika Westerberg has reported that the fixed+improved divisor based baud > >>> rate > >>> encoding method doesn't work anymore with his HXD device. > >>> So until we've found out what's going on, reintroduce the old encoding > >>> algorithm > >>> and use it for this and all newer chips for baud rates > 115200 baud. > >>> Also switch back to the direct encoding method for baud rates <= 115200, > >>> because > >>> it's unclear if the old divisor based encoding algorithm works for them. > >>> > >>> Reported-by: Mika Westerberg > >>> Signed-off-by: Frank Schäfer > >> Tried this and with 115200 it works and fixes the problem. However, with > >> speeds like 230400 and 460800 it still corrupts data. > > Well, with this patch we go back to what we did since kernel 3.1 (since > > commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above > > 115200"), so you should face the same problems with these kernels, too. > I've double checked that. With this patch applied to 3.12-rc the baud > rate encoding is exactly the same as in 3.11 (for HXD chips). > > Are you sure your test setup is reliable / comparable ? Could you please > re-check your results ? I re-tested with 3.11 and 3.12-rc7 with this patch applied. With 3.11 all tried speeds (115200, 230400 and 460800) work fine. With 3.12-rc7 + this patch, 115200 works but 230400 and 460800 corrupt data. My test setup is such that I just start getty on ttyUSB0 with given speed and then connect to it using picocom. Once logged in, command like 'ls' will return listing that is corrupted somehow: [root@tbt02 /]# ls NH$�j�I[0m/ init*linuxrc@ opt/ run@ tmp/r/ etc/ lib/ media/ proc/sbin/usr/ It is supposed to look like: [root@tbt02 /]# ls bin/ home/lib32@ mnt/ root/sys/ var/ dev/ init*linuxrc@ opt/ run@ tmp/ etc/ lib/ media/ proc/sbin/usr/ -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 29.10.2013 18:12, schrieb Frank Schäfer: > Am 29.10.2013 10:07, schrieb Mika Westerberg: >> On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: >>> Mika Westerberg has reported that the fixed+improved divisor based baud >>> rate >>> encoding method doesn't work anymore with his HXD device. >>> So until we've found out what's going on, reintroduce the old encoding >>> algorithm >>> and use it for this and all newer chips for baud rates > 115200 baud. >>> Also switch back to the direct encoding method for baud rates <= 115200, >>> because >>> it's unclear if the old divisor based encoding algorithm works for them. >>> >>> Reported-by: Mika Westerberg >>> Signed-off-by: Frank Schäfer >> Tried this and with 115200 it works and fixes the problem. However, with >> speeds like 230400 and 460800 it still corrupts data. > Well, with this patch we go back to what we did since kernel 3.1 (since > commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above > 115200"), so you should face the same problems with these kernels, too. I've double checked that. With this patch applied to 3.12-rc the baud rate encoding is exactly the same as in 3.11 (for HXD chips). Are you sure your test setup is reliable / comparable ? Could you please re-check your results ? Regards, Frank > > Please note that there are serious doubts with regards to the baud rate > precision of this algorithm and if it has really ever been working with > all chips. > The commit description doesn't say where it comes from (likey based on > reverse-enginnering) and which chips it tries to fix. :( We've contacted > the author, but got no reply. > With HX chips, it only worked "more or less" for baud rates ~115200 to > ~50 and but the actual baud rate always differed a little. > With commit 57ce61aad748 we fixed/improved this algorithm, but > apparently this doesn't work for HXD chips (which is a bit surprising). > > I hope I'll get one of these devices at the end of this week. > >> I also got few whitespace warnings when applied this using 'git am'. > Ick, thanks. Will send a fixed version. > > Regards, > Frank > -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Am 29.10.2013 10:07, schrieb Mika Westerberg: > On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: >> Mika Westerberg has reported that the fixed+improved divisor based baud rate >> encoding method doesn't work anymore with his HXD device. >> So until we've found out what's going on, reintroduce the old encoding >> algorithm >> and use it for this and all newer chips for baud rates > 115200 baud. >> Also switch back to the direct encoding method for baud rates <= 115200, >> because >> it's unclear if the old divisor based encoding algorithm works for them. >> >> Reported-by: Mika Westerberg >> Signed-off-by: Frank Schäfer > Tried this and with 115200 it works and fixes the problem. However, with > speeds like 230400 and 460800 it still corrupts data. Well, with this patch we go back to what we did since kernel 3.1 (since commit 8d48fdf689fe "USB: PL2303: correctly handle baudrates above 115200"), so you should face the same problems with these kernels, too. Please note that there are serious doubts with regards to the baud rate precision of this algorithm and if it has really ever been working with all chips. The commit description doesn't say where it comes from (likey based on reverse-enginnering) and which chips it tries to fix. :( We've contacted the author, but got no reply. With HX chips, it only worked "more or less" for baud rates ~115200 to ~50 and but the actual baud rate always differed a little. With commit 57ce61aad748 we fixed/improved this algorithm, but apparently this doesn't work for HXD chips (which is a bit surprising). I hope I'll get one of these devices at the end of this week. > I also got few whitespace warnings when applied this using 'git am'. Ick, thanks. Will send a fixed version. Regards, Frank -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
On Mon, Oct 28, 2013 at 07:50:44PM +0100, Frank Schäfer wrote: > Mika Westerberg has reported that the fixed+improved divisor based baud rate > encoding method doesn't work anymore with his HXD device. > So until we've found out what's going on, reintroduce the old encoding > algorithm > and use it for this and all newer chips for baud rates > 115200 baud. > Also switch back to the direct encoding method for baud rates <= 115200, > because > it's unclear if the old divisor based encoding algorithm works for them. > > Reported-by: Mika Westerberg > Signed-off-by: Frank Schäfer Tried this and with 115200 it works and fixes the problem. However, with speeds like 230400 and 460800 it still corrupts data. I also got few whitespace warnings when applied this using 'git am'. -- 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] pl2303: restore the old baud rate encoding for HXD (and newer) chips
Mika Westerberg has reported that the fixed+improved divisor based baud rate
encoding method doesn't work anymore with his HXD device.
So until we've found out what's going on, reintroduce the old encoding algorithm
and use it for this and all newer chips for baud rates > 115200 baud.
Also switch back to the direct encoding method for baud rates <= 115200, because
it's unclear if the old divisor based encoding algorithm works for them.
Reported-by: Mika Westerberg
Signed-off-by: Frank Schäfer
---
drivers/usb/serial/pl2303.c | 64 +++
1 Datei geändert, 58 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index bedf8e4..32f0410 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -372,14 +372,14 @@ static int pl2303_baudrate_encode_direct(int baud, enum
pl2303_type type,
return baud;
}
-static int pl2303_baudrate_encode_divisor(int baud, enum pl2303_type type,
+static int pl2303_baudrate_encode_divisor_HX(int baud, enum pl2303_type type,
u8 buf[4])
{
/*
-* Divisor based baud rate encoding method
+* Divisor based baud rate encoding method for HX chips
*
* NOTE: HX clones do NOT support this method.
-* It's not clear if the type_0/1 chips support it.
+* It's not clear if other chips support it.
*
* divisor = 12MHz * 32 / baudrate = 2^A * B
*
@@ -456,6 +456,53 @@ static int pl2303_baudrate_encode_divisor(int baud, enum
pl2303_type type,
return baud;
}
+static int pl2303_baudrate_encode_divisor_HXD(int baud, enum pl2303_type type,
+ u8 buf[4])
+{
+ /*
+* Divisor based baud rate encoding method for HXD chips
+*
+* NOTE: This method has been used since kernel 3.1 for all chips and
+* baud rates > 115200. It is very similar to the new (and better)
+* method for HX chips, but it has been reported that the HX method
+* doesn't work at least for HXD chips and baud rates > 115200.
+*/
+ unsigned tmp;
+
+ /*
+* NOTE: The Windows driver allows maximum baud rates of 110% of the
+* specified maximium value.
+* Quick tests with early (2004) HX (rev. A) chips suggest, that even
+* higher baud rates (up to the maximum of 24M baud !) are working fine,
+* but that should really be tested carefully in "real life" scenarios
+* before removing the upper limit completely.
+*
+* Specified max. baud rates:
+* HXD, EA, TB: 12Mbps; RA: 1Mbps; SA: 115200 bps
+*
+* FIXME: as long as we don't know how to distinguish between
+* these chip variants, allow the max. of these values
+*/
+ baud = min_t(int, baud, 1200 * 1.1);
+
+ /* apparently the formula for higher speeds is:
+* baudrate = 12M * 32 / (2^buf[1]) / buf[0]
+*/
+ tmp = 12*1000*1000*32 / baud;
+ buf[3] = 0x80;
+ buf[2] = 0;
+ buf[1] = (tmp >= 256);
+ while (tmp >= 256) {
+ tmp >>= 2;
+ buf[1] <<= 1;
+ }
+ buf[0] = tmp;
+ /* Calculate the actual/resulting baud rate */
+ baud = 1200 * 32 / ((1 << buf[1]) * buf[0]);
+
+ return baud;
+}
+
static void pl2303_encode_baudrate(struct tty_struct *tty,
struct usb_serial_port *port,
enum pl2303_type type,
@@ -473,6 +520,9 @@ static void pl2303_encode_baudrate(struct tty_struct *tty,
*=> supported by all chip types
* 2) Divisor based method: encodes a divisor to a base value (12MHz*32)
*=> not supported by HX clones (and likely type_0/1 chips)
+*=> supported by HX chips for the whole baud rate range
+*=> HXD (and likely also EA, RA, SA, TB chips) support this method
+* only for baud rates > 115200
*
* NOTE: Although the divisor based baud rate encoding method is much
* more flexible, some of the standard baud rate values can not be
@@ -480,10 +530,12 @@ static void pl2303_encode_baudrate(struct tty_struct *tty,
* the device likely uses the same baud rate generator for both methods
* so that there is likley no difference.
*/
- if (type == type_0 || type == type_1 || type == HX_CLONE)
- baud = pl2303_baudrate_encode_direct(baud, type, buf);
+ if (type == HX_TA)
+ baud = pl2303_baudrate_encode_divisor_HX(baud, type, buf);
+ else if ((type == HXD_EA_RA_SA || type == TB) && baud > 115200)
+ baud = pl2303_baudrate_encode_divisor_HXD(baud, type, buf);
else
- baud = pl2303_baudrate_encode_
