| Message ID | 20211231171516.18407-1-jochen@mades.net |
|---|---|
| State | New |
| Headers | show |
| Series | Bugfix RTS line config in RS485 mode is overwritten in pl011_set_mctrl() function. | expand |
On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote: > Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device. > > Signed-off-by: jmades <jochen@mades.net> Patch is correct, but commit message could be improved: * Subject should be in imperative mood (by convention), it should be prepended by "serial: pl011: " (in line with previous commits touching this driver, use "git log --oneline amba-pl011.c") and the trailing dot is unnecessary, e.g.: "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl" * Commit message should be wrapped at 72 characters (so that it appears centered when displayed with "git log" on an 80 chars terminal). The reference to "0001-serial-amba-pl011-add-RS485-support.patch" should be replaced with a reference to the offending commit, e.g.: "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought to keep RTS deasserted on set_mctrl if rs485 is enabled. However it did so only if deasserted RTS polarity is high. Fix it in case it's low." Feel free to copy this to a v2 of your patch and amend as you see fit. * Add tags for the offending commit: Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support") Cc: stable@vger.kernel.org # v5.15+ * Be sure to cc the author of the offending commit. Thanks, Lukas > --- > drivers/tty/serial/amba-pl011.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 537f37ac4..1749c1498 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) > container_of(port, struct uart_amba_port, port); > unsigned int cr; > > - if (port->rs485.flags & SER_RS485_ENABLED) > - mctrl &= ~TIOCM_RTS; > + if (port->rs485.flags & SER_RS485_ENABLED) { > + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) > + mctrl &= ~TIOCM_RTS; > + else > + mctrl |= TIOCM_RTS; > + } > > cr = pl011_read(uap, REG_CR);
Hi, On 02.01.22 at 11:07, Lukas Wunner wrote: > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote: >> Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device. >> >> Signed-off-by: jmades <jochen@mades.net> > > Patch is correct, but commit message could be improved: > > * Subject should be in imperative mood (by convention), it should be > prepended by "serial: pl011: " (in line with previous commits touching > this driver, use "git log --oneline amba-pl011.c") and the trailing dot > is unnecessary, e.g.: > > "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl" > > * Commit message should be wrapped at 72 characters (so that it appears > centered when displayed with "git log" on an 80 chars terminal). > The reference to "0001-serial-amba-pl011-add-RS485-support.patch" > should be replaced with a reference to the offending commit, e.g.: > > "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought > to keep RTS deasserted on set_mctrl if rs485 is enabled. However it > did so only if deasserted RTS polarity is high. Fix it in case it's > low." > > Feel free to copy this to a v2 of your patch and amend as you see fit. > > * Add tags for the offending commit: > > Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support") > Cc: stable@vger.kernel.org # v5.15+ > > * Be sure to cc the author of the offending commit. > > Thanks, > > Lukas > >> --- >> drivers/tty/serial/amba-pl011.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c >> index 537f37ac4..1749c1498 100644 >> --- a/drivers/tty/serial/amba-pl011.c >> +++ b/drivers/tty/serial/amba-pl011.c >> @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) >> container_of(port, struct uart_amba_port, port); >> unsigned int cr; >> >> - if (port->rs485.flags & SER_RS485_ENABLED) >> - mctrl &= ~TIOCM_RTS; >> + if (port->rs485.flags & SER_RS485_ENABLED) { >> + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) >> + mctrl &= ~TIOCM_RTS; >> + else >> + mctrl |= TIOCM_RTS; >> + } >> >> cr = pl011_read(uap, REG_CR); Does this logic really have to be implemented in the driver? It looks as if the serial core already takes RS485 into account before calling set_mctrls(). At least I get the impression when looking at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx simply seem to ignore RTS in case of RS485. Regards, Lino
On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote: > On 02.01.22 at 11:07, Lukas Wunner wrote: > > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote: > > > --- a/drivers/tty/serial/amba-pl011.c > > > +++ b/drivers/tty/serial/amba-pl011.c > > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > container_of(port, struct uart_amba_port, port); > > > unsigned int cr; > > > > > > - if (port->rs485.flags & SER_RS485_ENABLED) > > > - mctrl &= ~TIOCM_RTS; > > > + if (port->rs485.flags & SER_RS485_ENABLED) { > > > + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) > > > + mctrl &= ~TIOCM_RTS; > > > + else > > > + mctrl |= TIOCM_RTS; > > > + } > > > > > > cr = pl011_read(uap, REG_CR); > > Does this logic really have to be implemented in the driver? No, it doesn't have to be and indeed I'm working towards consolidating it in the serial core with this collection of patches: https://git.kernel.org/gregkh/tty/c/d3b3404df318 https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de https://github.com/l1k/linux/commit/532ef2ad757f The last of these removes the rs485 logic from pl011_set_mctrl(). I'll post it once the others (and Jochen Mades' patch) have landed. Even though the logic is eventually removed from pl011_set_mctrl(), Jochen's patch makes sense as a backportable fix for v5.15. > It looks as if the serial core already takes RS485 into account before > calling set_mctrls(). At least I get the impression when looking > at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx > simply seem to ignore RTS in case of RS485. The logic in uart_port_dtr_rts() is broken. That's fixed by d3b3404df318, which is queued up in tty-next for v5.17. The pl011 driver papered over it with its own rs485-specific logic in pl011_set_mctrl(). But as Jochen Mades correctly pointed out, that only worked correctly if RTS is driven high on idle. The logic in uart_tiocmset() is correct, but not sufficient because uart_throttle(), uart_unthrottle and uart_set_termios() need to become rs485-aware as well. That's also addressed by the above-linked GitHub commit. Thanks, Lukas
On 02.01.22 at 19:28, Lukas Wunner wrote: > On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote: >> On 02.01.22 at 11:07, Lukas Wunner wrote: >>> On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote: >>>> --- a/drivers/tty/serial/amba-pl011.c >>>> +++ b/drivers/tty/serial/amba-pl011.c >>>> @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) >>>> container_of(port, struct uart_amba_port, port); >>>> unsigned int cr; >>>> >>>> - if (port->rs485.flags & SER_RS485_ENABLED) >>>> - mctrl &= ~TIOCM_RTS; >>>> + if (port->rs485.flags & SER_RS485_ENABLED) { >>>> + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) >>>> + mctrl &= ~TIOCM_RTS; >>>> + else >>>> + mctrl |= TIOCM_RTS; >>>> + } >>>> >>>> cr = pl011_read(uap, REG_CR); >> >> Does this logic really have to be implemented in the driver? > > No, it doesn't have to be and indeed I'm working towards consolidating > it in the serial core with this collection of patches: > > https://git.kernel.org/gregkh/tty/c/d3b3404df318 > https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de > https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de > https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de > https://github.com/l1k/linux/commit/532ef2ad757f > > The last of these removes the rs485 logic from pl011_set_mctrl(). > I'll post it once the others (and Jochen Mades' patch) have landed. > > Even though the logic is eventually removed from pl011_set_mctrl(), > Jochen's patch makes sense as a backportable fix for v5.15. > > >> It looks as if the serial core already takes RS485 into account before >> calling set_mctrls(). At least I get the impression when looking >> at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx >> simply seem to ignore RTS in case of RS485. > > The logic in uart_port_dtr_rts() is broken. That's fixed by d3b3404df318, > which is queued up in tty-next for v5.17. > > The pl011 driver papered over it with its own rs485-specific logic in > pl011_set_mctrl(). But as Jochen Mades correctly pointed out, that > only worked correctly if RTS is driven high on idle. > > > The logic in uart_tiocmset() is correct, but not sufficient because > uart_throttle(), uart_unthrottle and uart_set_termios() need to become > rs485-aware as well. That's also addressed by the above-linked > GitHub commit. > Thanks for this information. I have one or two patches prepared a while ago that aim into a similar direction (move RS485 related code out of the drivers into the serial core). I will send them in the next days. Best regards, Lino
Hi Lukas, Lino, please let me know when I could test again with an "official" linux kernel, instead of using my local patch. Bests Jochen > On 02/01/2022 19:28 Lukas Wunner <lukas@wunner.de> wrote: > > > On Sun, Jan 02, 2022 at 04:06:53PM +0100, Lino Sanfilippo wrote: > > On 02.01.22 at 11:07, Lukas Wunner wrote: > > > On Fri, Dec 31, 2021 at 05:15:14PM +0000, jmades wrote: > > > > --- a/drivers/tty/serial/amba-pl011.c > > > > +++ b/drivers/tty/serial/amba-pl011.c > > > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > > container_of(port, struct uart_amba_port, port); > > > > unsigned int cr; > > > > > > > > - if (port->rs485.flags & SER_RS485_ENABLED) > > > > - mctrl &= ~TIOCM_RTS; > > > > + if (port->rs485.flags & SER_RS485_ENABLED) { > > > > + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) > > > > + mctrl &= ~TIOCM_RTS; > > > > + else > > > > + mctrl |= TIOCM_RTS; > > > > + } > > > > > > > > cr = pl011_read(uap, REG_CR); > > > > Does this logic really have to be implemented in the driver? > > No, it doesn't have to be and indeed I'm working towards consolidating > it in the serial core with this collection of patches: > > https://git.kernel.org/gregkh/tty/c/d3b3404df318 > https://lore.kernel.org/all/f49f945375f5ccb979893c49f1129f51651ac738.1641129062.git.lukas@wunner.de > https://lore.kernel.org/all/e22089ab49e6e78822c50c8c4db46bf3ee885623.1641129328.git.lukas@wunner.de > https://lore.kernel.org/all/bceeaba030b028ed810272d55d5fc6f3656ddddb.1641129752.git.lukas@wunner.de > https://github.com/l1k/linux/commit/532ef2ad757f > > The last of these removes the rs485 logic from pl011_set_mctrl(). > I'll post it once the others (and Jochen Mades' patch) have landed. > > Even though the logic is eventually removed from pl011_set_mctrl(), > Jochen's patch makes sense as a backportable fix for v5.15. > > > > It looks as if the serial core already takes RS485 into account before > > calling set_mctrls(). At least I get the impression when looking > > at uart_tiocmset() and uart_port_dtr_rts(). Also other drivers like imx > > simply seem to ignore RTS in case of RS485. > > The logic in uart_port_dtr_rts() is broken. That's fixed by d3b3404df318, > which is queued up in tty-next for v5.17. > > The pl011 driver papered over it with its own rs485-specific logic in > pl011_set_mctrl(). But as Jochen Mades correctly pointed out, that > only worked correctly if RTS is driven high on idle. > > > The logic in uart_tiocmset() is correct, but not sufficient because > uart_throttle(), uart_unthrottle and uart_set_termios() need to become > rs485-aware as well. That's also addressed by the above-linked > GitHub commit. > > Thanks, > > Lukas
Hi Jochen, On 11.01.22 at 11:00, Jochen Mades wrote: > Hi Lukas, Lino, > > please let me know when I could test again with an "official" linux kernel, instead of using my local patch. > > Bests > Jochen While the fix itself looks good so far there are still some style issue as Lukas pointed out. These issues have to be fixed before the patch can be accepted for mainline integration. Please have a look at Documentation/process/submitting-patches.rst for the correct patch format. After these issues are fixed please send the patch as a v2, so we can review the new version. Best regards, Lino
On Thu, Jan 13, 2022 at 11:12:12AM +0100, Jochen Mades wrote: > Hi Lukas, > > > Patch is correct, but commit message could be improved: > > > > * Subject should be in imperative mood (by convention), it should be > > prepended by "serial: pl011: " (in line with previous commits touching > > this driver, use "git log --oneline amba-pl011.c") and the trailing dot > > is unnecessary, e.g.: > > > > "serial: pl011: Fix incorrect rs485 RTS polarity on set_mctrl" > > > > * Commit message should be wrapped at 72 characters (so that it appears > > centered when displayed with "git log" on an 80 chars terminal). > > The reference to "0001-serial-amba-pl011-add-RS485-support.patch" > > should be replaced with a reference to the offending commit, e.g.: > > > > "Commit 8d479237727c ("serial: amba-pl011: add RS485 support") sought > > to keep RTS deasserted on set_mctrl if rs485 is enabled. However it > > did so only if deasserted RTS polarity is high. Fix it in case it's > > low." > > > > Feel free to copy this to a v2 of your patch and amend as you see fit. > > > > Find attached the patch with the new subject and corretced commit message. > > > * Add tags for the offending commit: > > > > Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support") > > Cc: stable@vger.kernel.org # v5.15+ > > > > * Be sure to cc the author of the offending commit. > > Sorry I don't know how to do that correctly. Can you please give support/hints? > > > > Thanks, > > > > Lukas > > > > > --- > > > drivers/tty/serial/amba-pl011.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > > > index 537f37ac4..1749c1498 100644 > > > --- a/drivers/tty/serial/amba-pl011.c > > > +++ b/drivers/tty/serial/amba-pl011.c > > > @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) > > > container_of(port, struct uart_amba_port, port); > > > unsigned int cr; > > > > > > - if (port->rs485.flags & SER_RS485_ENABLED) > > > - mctrl &= ~TIOCM_RTS; > > > + if (port->rs485.flags & SER_RS485_ENABLED) { > > > + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) > > > + mctrl &= ~TIOCM_RTS; > > > + else > > > + mctrl |= TIOCM_RTS; > > > + } > > > > > > cr = pl011_read(uap, REG_CR); Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch was attached, please place it inline so that it can be applied directly from the email message itself. - It looks like you did not use your "real" name for the patch on either the Signed-off-by: line, or the From: line (both of which have to match). Please read the kernel file, Documentation/SubmittingPatches for how to do this correctly. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 537f37ac4..1749c1498 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -1646,8 +1646,12 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl) container_of(port, struct uart_amba_port, port); unsigned int cr; - if (port->rs485.flags & SER_RS485_ENABLED) - mctrl &= ~TIOCM_RTS; + if (port->rs485.flags & SER_RS485_ENABLED) { + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND) + mctrl &= ~TIOCM_RTS; + else + mctrl |= TIOCM_RTS; + } cr = pl011_read(uap, REG_CR);
Based on the "0001-serial-amba-pl011-add-RS485-support.patch" this change is necesarry otherwise the RTS-line will be pulled up in SER_RS485_RTS_BEFORE_SEND mode before sending data. This hinders the driver to receive data, f.ex. when the device is an RS485 slave device. Signed-off-by: jmades <jochen@mades.net> --- drivers/tty/serial/amba-pl011.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)