According to the lpuart reference manual, need to writing a 1 and then a
0 to the UARTCTRL_SBK field queues a break character in the transmit
data stream. Only writing a 1 cannot trigger the break character, so fix
it.
Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
Signed-off-by: Sherry Sun <[email protected]>
---
drivers/tty/serial/fsl_lpuart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index b6365566a460..8a4aae7dbd99 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1486,7 +1486,7 @@ static void lpuart32_break_ctl(struct uart_port *port, int break_state)
temp = lpuart32_read(port, UARTCTRL) & ~UARTCTRL_SBK;
if (break_state != 0)
- temp |= UARTCTRL_SBK;
+ lpuart32_write(port, temp | UARTCTRL_SBK, UARTCTRL);
lpuart32_write(port, temp, UARTCTRL);
}
--
2.17.1
Hi,
Am 2022-07-15 04:59, schrieb Sherry Sun:
> According to the lpuart reference manual, need to writing a 1 and then
> a
> 0 to the UARTCTRL_SBK field queues a break character in the transmit
> data stream. Only writing a 1 cannot trigger the break character, so
> fix
> it.
I don't think this is correct. The tty core will already call this:
.break_ctl(port, 1)
usleep()
.break_ctl(port, 0)
So you'll have your 1->0 transition.
My RM from the LS1028A says the following:
| Writing a 1 and then a 0 to SBK queues a break character in the
| transmit data stream. Additional break characters of 10 to 13,
| or 13 to 16 if LPUART_STATBRK13] is set, bit times of logic 0
| are queued as long as SBK is set. Depending on the timing of
| the set and clear of SBK relative to the information currently
| being transmitted, a second break character may be queued
| before software clears SBK.
To me it seems that setting the SBK bit just pulls the TX line
low and releasing it will return to normal transmitter mode.
-michael
Hi,
Am 2022-07-15 09:20, schrieb Sherry Sun:
>> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then
>> a 0 to
>> trigger a break character
>>
>> Hi,
>>
>> Am 2022-07-15 04:59, schrieb Sherry Sun:
>> > According to the lpuart reference manual, need to writing a 1 and then
>> > a
>> > 0 to the UARTCTRL_SBK field queues a break character in the transmit
>> > data stream. Only writing a 1 cannot trigger the break character, so
>> > fix it.
>>
>> I don't think this is correct. The tty core will already call this:
>> .break_ctl(port, 1)
>> usleep()
>> .break_ctl(port, 0)
>>
>> So you'll have your 1->0 transition.
>>
>> My RM from the LS1028A says the following:
>>
>> | Writing a 1 and then a 0 to SBK queues a break character in the
>> | transmit data stream. Additional break characters of 10 to 13, or 13
>> | to 16 if LPUART_STATBRK13] is set, bit times of logic 0 are queued
>> as
>> | long as SBK is set. Depending on the timing of the set and clear of
>> | SBK relative to the information currently being transmitted, a
>> second
>> | break character may be queued before software clears SBK.
>>
>> To me it seems that setting the SBK bit just pulls the TX line low and
>> releasing
>> it will return to normal transmitter mode.
>>
>
> Hi Michael,
>
> Actually set break_ctl(tty, -1) then break_ctl(tty, 0) is only done in
> the send_break() function.
> If we call TIOCSBRK from user space, it will only set break_ctl(tty,
> -1) without break_ctl(tty, 0).
That is expected. no? There is also the TIOCCBRK which will clear the
break. TIOCSBRK will just turn the break on.
I'm not sure if the break is already transmitted when the SBK bit
is set, though. Is that your problem here? I'd need to check that
on the real hardware.
> And from the definition of .break_ctl(port,ctl), the callback is used
> to Control the transmission of a break
> signal(Documentation/driver-api/serial/driver.rst), if ctl is nonzero,
> it should queues a break character. I don't think it is reasonable to
> call break_ctl() twice in order to send one break signal.
Maybe Gred can correct me, but to me it seems like the .break_ctl()
will set the *state* according to the argument, that is either
turning it on or turning it off (Except if TTY_DRIVER_HARDWARE_BREAK
is set, but that doesn't seem to be supported by the ioctl interface.)
> Also I have tried other uart IP, such as drivers/tty/serial/imx.c, it
> also queues a break character if we call break_ctl() once. So I
> believe the break_ctl() in lpuart driver should be fixed.
-michael
> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
> trigger a break character
>
> Hi,
>
> Am 2022-07-15 04:59, schrieb Sherry Sun:
> > According to the lpuart reference manual, need to writing a 1 and then
> > a
> > 0 to the UARTCTRL_SBK field queues a break character in the transmit
> > data stream. Only writing a 1 cannot trigger the break character, so
> > fix it.
>
> I don't think this is correct. The tty core will already call this:
> .break_ctl(port, 1)
> usleep()
> .break_ctl(port, 0)
>
> So you'll have your 1->0 transition.
>
> My RM from the LS1028A says the following:
>
> | Writing a 1 and then a 0 to SBK queues a break character in the
> | transmit data stream. Additional break characters of 10 to 13, or 13
> | to 16 if LPUART_STATBRK13] is set, bit times of logic 0 are queued as
> | long as SBK is set. Depending on the timing of the set and clear of
> | SBK relative to the information currently being transmitted, a second
> | break character may be queued before software clears SBK.
>
> To me it seems that setting the SBK bit just pulls the TX line low and releasing
> it will return to normal transmitter mode.
>
Hi Michael,
Actually set break_ctl(tty, -1) then break_ctl(tty, 0) is only done in the send_break() function.
If we call TIOCSBRK from user space, it will only set break_ctl(tty, -1) without break_ctl(tty, 0).
And from the definition of .break_ctl(port,ctl), the callback is used to Control the transmission of a break signal(Documentation/driver-api/serial/driver.rst), if ctl is nonzero, it should queues a break character. I don't think it is reasonable to call break_ctl() twice in order to send one break signal.
Also I have tried other uart IP, such as drivers/tty/serial/imx.c, it also queues a break character if we call break_ctl() once. So I believe the break_ctl() in lpuart driver should be fixed.
Best regards
Sherry
> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
> trigger a break character
>
> Hi,
>
> Am 2022-07-15 09:20, schrieb Sherry Sun:
> >> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and
> >> then a 0 to trigger a break character
> >>
> >> Hi,
> >>
> >> Am 2022-07-15 04:59, schrieb Sherry Sun:
> >> > According to the lpuart reference manual, need to writing a 1 and
> >> > then a
> >> > 0 to the UARTCTRL_SBK field queues a break character in the
> >> > transmit data stream. Only writing a 1 cannot trigger the break
> >> > character, so fix it.
> >>
> >> I don't think this is correct. The tty core will already call this:
> >> .break_ctl(port, 1)
> >> usleep()
> >> .break_ctl(port, 0)
> >>
> >> So you'll have your 1->0 transition.
> >>
> >> My RM from the LS1028A says the following:
> >>
> >> | Writing a 1 and then a 0 to SBK queues a break character in the
> >> | transmit data stream. Additional break characters of 10 to 13, or
> >> | 13 to 16 if LPUART_STATBRK13] is set, bit times of logic 0 are
> >> | queued
> >> as
> >> | long as SBK is set. Depending on the timing of the set and clear of
> >> | SBK relative to the information currently being transmitted, a
> >> second
> >> | break character may be queued before software clears SBK.
> >>
> >> To me it seems that setting the SBK bit just pulls the TX line low
> >> and releasing it will return to normal transmitter mode.
> >>
> >
> > Hi Michael,
> >
> > Actually set break_ctl(tty, -1) then break_ctl(tty, 0) is only done in
> > the send_break() function.
> > If we call TIOCSBRK from user space, it will only set break_ctl(tty,
> > -1) without break_ctl(tty, 0).
>
> That is expected. no? There is also the TIOCCBRK which will clear the
> break. TIOCSBRK will just turn the break on.
>
> I'm not sure if the break is already transmitted when the SBK bit
> is set, though. Is that your problem here? I'd need to check that
> on the real hardware.
>
Hi Michael,
Seems we have the different understanding of the break_ctl(port,ctl) callback. The original break_ctl(tty,-1) in lpuart will not send the break signal until we call break_ctl(tty,0). Per my understanding of "If ctl is nonzero, the break signal should be transmitted", call break_ctl(tty,-1) is enough the send one break signal, now my patch makes the behavior align with my understanding.
And my understanding of break_ctl(tty,0) is that it will terminate the break signal send requirement which has not been done instead of cooperate with break_ctl(tty,-1) to finish one break character send behavior.
| break_ctl(port,ctl)
| Control the transmission of a break signal. If ctl is
| nonzero, the break signal should be transmitted. The signal
| should be terminated when another call is made with a zero
| ctl.
Best regards
Sherry
> > And from the definition of .break_ctl(port,ctl), the callback is used
> > to Control the transmission of a break
> > signal(Documentation/driver-api/serial/driver.rst), if ctl is nonzero,
> > it should queues a break character. I don't think it is reasonable to
> > call break_ctl() twice in order to send one break signal.
>
> Maybe Gred can correct me, but to me it seems like the .break_ctl()
> will set the *state* according to the argument, that is either
> turning it on or turning it off (Except if TTY_DRIVER_HARDWARE_BREAK
> is set, but that doesn't seem to be supported by the ioctl interface.)
>
> > Also I have tried other uart IP, such as drivers/tty/serial/imx.c, it
> > also queues a break character if we call break_ctl() once. So I
> > believe the break_ctl() in lpuart driver should be fixed.
>
> -michael
Am 2022-07-15 11:18, schrieb Sherry Sun:
>> Am 2022-07-15 09:20, schrieb Sherry Sun:
>> >> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and
>> >> then a 0 to trigger a break character
>> >>
>> >> Hi,
>> >>
>> >> Am 2022-07-15 04:59, schrieb Sherry Sun:
>> >> > According to the lpuart reference manual, need to writing a 1 and
>> >> > then a
>> >> > 0 to the UARTCTRL_SBK field queues a break character in the
>> >> > transmit data stream. Only writing a 1 cannot trigger the break
>> >> > character, so fix it.
>> >>
>> >> I don't think this is correct. The tty core will already call this:
>> >> .break_ctl(port, 1)
>> >> usleep()
>> >> .break_ctl(port, 0)
>> >>
>> >> So you'll have your 1->0 transition.
>> >>
>> >> My RM from the LS1028A says the following:
>> >>
>> >> | Writing a 1 and then a 0 to SBK queues a break character in the
>> >> | transmit data stream. Additional break characters of 10 to 13, or
>> >> | 13 to 16 if LPUART_STATBRK13] is set, bit times of logic 0 are
>> >> | queued
>> >> as
>> >> | long as SBK is set. Depending on the timing of the set and clear of
>> >> | SBK relative to the information currently being transmitted, a
>> >> second
>> >> | break character may be queued before software clears SBK.
>> >>
>> >> To me it seems that setting the SBK bit just pulls the TX line low
>> >> and releasing it will return to normal transmitter mode.
>> >>
>> >
>> > Hi Michael,
>> >
>> > Actually set break_ctl(tty, -1) then break_ctl(tty, 0) is only done in
>> > the send_break() function.
>> > If we call TIOCSBRK from user space, it will only set break_ctl(tty,
>> > -1) without break_ctl(tty, 0).
>>
>> That is expected. no? There is also the TIOCCBRK which will clear the
>> break. TIOCSBRK will just turn the break on.
>>
>> I'm not sure if the break is already transmitted when the SBK bit
>> is set, though. Is that your problem here? I'd need to check that
>> on the real hardware.
>>
>
> Hi Michael,
>
> Seems we have the different understanding of the break_ctl(port,ctl)
> callback. The original break_ctl(tty,-1) in lpuart will not send the
> break signal until we call break_ctl(tty,0).
That is not correct. The TX linue goes low as soon as the SBK bit
is set. See below.
> Per my understanding of
> "If ctl is nonzero, the break signal should be transmitted", call
> break_ctl(tty,-1) is enough the send one break signal, now my patch
> makes the behavior align with my understanding.
As I said, Greg should clarify here.
In any case, I've checked the hardware (LS1028A) and as soon as you
set the SBK bit, the TX line goes low (TTL levels) as expected. It
will go to high again as soon as you clear the bit again.
So to me it seems there is nothing wrong here. Also have a look
at man ioctl_tty:
TIOCSBRK
Turn break on, that is, start sending zero bits.
TIOCCBRK
Turn break off, that is, stop sending zero bits.
So to send one break "character", you need to do ioctl(TIOCSBRK)
followed by an ioctl(TIOCCBRK).
-michael
> And my understanding of break_ctl(tty,0) is that it will terminate the
> break signal send requirement which has not been done instead of
> cooperate with break_ctl(tty,-1) to finish one break character send
> behavior.
>
> | break_ctl(port,ctl)
> | Control the transmission of a break signal. If ctl is
> | nonzero, the break signal should be transmitted. The signal
> | should be terminated when another call is made with a zero
> | ctl.
>
> Best regards
> Sherry
>
>
>> > And from the definition of .break_ctl(port,ctl), the callback is used
>> > to Control the transmission of a break
>> > signal(Documentation/driver-api/serial/driver.rst), if ctl is nonzero,
>> > it should queues a break character. I don't think it is reasonable to
>> > call break_ctl() twice in order to send one break signal.
>>
>> Maybe Gred can correct me, but to me it seems like the .break_ctl()
>> will set the *state* according to the argument, that is either
>> turning it on or turning it off (Except if TTY_DRIVER_HARDWARE_BREAK
>> is set, but that doesn't seem to be supported by the ioctl interface.)
>>
>> > Also I have tried other uart IP, such as drivers/tty/serial/imx.c, it
>> > also queues a break character if we call break_ctl() once. So I
>> > believe the break_ctl() in lpuart driver should be fixed.
On 15. 07. 22, 13:52, Michael Walle wrote:
>> Seems we have the different understanding of the break_ctl(port,ctl)
>> callback. The original break_ctl(tty,-1) in lpuart will not send the
>> break signal until we call break_ctl(tty,0).
>
> That is not correct. The TX linue goes low as soon as the SBK bit
> is set. See below.
In that case…
>> Per my understanding of
>> "If ctl is nonzero, the break signal should be transmitted", call
>> break_ctl(tty,-1) is enough the send one break signal, now my patch
>> makes the behavior align with my understanding.
>
> As I said, Greg should clarify here.
>
> In any case, I've checked the hardware (LS1028A) and as soon as you
> set the SBK bit, the TX line goes low (TTL levels) as expected. It
> will go to high again as soon as you clear the bit again.
>
> So to me it seems there is nothing wrong here. Also have a look
> at man ioctl_tty:
>
> TIOCSBRK
> Turn break on, that is, start sending zero bits.
>
> TIOCCBRK
> Turn break off, that is, stop sending zero bits.
>
> So to send one break "character", you need to do ioctl(TIOCSBRK)
> followed by an ioctl(TIOCCBRK).
… you're right.
regards,
--
js
> On 15. 07. 22, 13:52, Michael Walle wrote:
> >> Seems we have the different understanding of the break_ctl(port,ctl)
> >> callback. The original break_ctl(tty,-1) in lpuart will not send the
> >> break signal until we call break_ctl(tty,0).
> >
> > That is not correct. The TX linue goes low as soon as the SBK bit is
> > set. See below.
>
> In that case…
>
> >> Per my understanding of
> >> "If ctl is nonzero, the break signal should be transmitted", call
> >> break_ctl(tty,-1) is enough the send one break signal, now my patch
> >> makes the behavior align with my understanding.
> >
> > As I said, Greg should clarify here.
> >
> > In any case, I've checked the hardware (LS1028A) and as soon as you
> > set the SBK bit, the TX line goes low (TTL levels) as expected. It
> > will go to high again as soon as you clear the bit again.
> >
> > So to me it seems there is nothing wrong here. Also have a look at man
> > ioctl_tty:
> >
> > TIOCSBRK
> > Turn break on, that is, start sending zero bits.
> >
> > TIOCCBRK
> > Turn break off, that is, stop sending zero bits.
> >
> > So to send one break "character", you need to do ioctl(TIOCSBRK)
> > followed by an ioctl(TIOCCBRK).
>
> … you're right.
Hi Jiri and Michael,
Per the understanding of the ioctl behavior here, does that mean the uart ops break_ctl(port,ctl) also perform the same behavior with ioctl_tty?
break_ctl(port,-1) will turn the break on, which may keep TX line low until we call break_ctl(port,0) to turn break off, which may raise the TX line, right?
If this is the correct understanding of break_ctl(port,ctl), then I will drop this patch, since my patch here cause the TX line only send one break signal(9 to 13 bits, or 12 to 15 bits if STAT[BRK13] is set). Thanks!
Best regards
Sherry
>
> regards,
> --
> js