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 <[email protected]>
---
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);
--
2.20.1
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 <[email protected]>
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: [email protected] # 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 <[email protected]>
>
> 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: [email protected] # 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 <[email protected]> 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
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: [email protected] # 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);
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: [email protected] # 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
On Thu, Jan 13, 2022 at 11:12:12AM +0100, Jochen Mades wrote:
> Find attached the patch with the new subject and corretced commit message.
Greg doesn't accept patches as attachments and during the merge window
(which ends today). I've just resubmitted your patch as part of a
2-patch series, together with a patch of mine which goes on top of yours:
https://lore.kernel.org/linux-serial/85fa3323ba8c307943969b7343e23f34c3e652ba.1642909284.git.lukas@wunner.de/
(I sent your patch twice because I botched the Date header the first time
around, sorry about that.)
Note that you still get patch authorship (and thus credit for the patch)
even though I submitted it, due to the From: line at the top of the
message body.
> > * Add tags for the offending commit:
> >
> > Fixes: 8d479237727c ("serial: amba-pl011: add RS485 support")
> > Cc: [email protected] # 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?
It just means that you need to add the two above-quoted tags to the
commit message. The maintainers of stable kernels use them to
determine which patches need to be backported to which kernel
releases. I've fixed up the commit message of your patch with
the missing tags, no problem.
And "cc the author of the offending commit" just means that when
posting the patch, it is customary to send a copy of the e-mail
to the author of the commit you're fixing, which is Lino in this
case, as he authored 8d479237727c.
Thanks,
Lukas