Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Sherry Sun <sherry.sun@nxp.com>
To:     Michael Walle <michael@walle.cc>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "jirislaby@kernel.org" <jirislaby@kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
 trigger a break character
Thread-Topic: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a 0 to
 trigger a break character
Thread-Index: AQHYl/cqr4tzMlicFUS5QAVEdKaVoa1+/W8AgAAE7pCAAAp2gIAAEHKQ
Date:   Fri, 15 Jul 2022 09:18:31 +0000
Message-ID: <AS8PR04MB8404B8E3EB0FFCEE8ADDA283928B9@AS8PR04MB8404.eurprd04.prod.outlook.com>
References: <20220715025944.11076-1-sherry.sun@nxp.com>
 <20220715025944.11076-3-sherry.sun@nxp.com>
 <509669b26b5899088e9b77ed94d103ee@walle.cc>
 <AS8PR04MB840448675E64E4FCDEEF91A1928B9@AS8PR04MB8404.eurprd04.prod.outlook.com>
 <e2560f01fd1731ea2422d82c97efcc6f@walle.cc>
In-Reply-To: <e2560f01fd1731ea2422d82c97efcc6f@walle.cc>
Accept-Language: zh-CN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ZPM0lffiJ56UB7uAhrWJNlGd9d99GG09n+IrmArnF9j0ujhvU4jAeWvPmKFU?=
 =?us-ascii?Q?1K2IYag+sh1ttrFxxJ8PCyujfgN+Pl19xfLU2GTfNOT12I+OcxA1vgjZAFDu?=
 =?us-ascii?Q?mIxW2V2C7/va3JgAS7vPgKBedgjFjmVbJHvxwMKvfo6+xrlw86nOsHPjCZww?=
 =?us-ascii?Q?c4yYZODI6Y0Fp4cIxEF7TRW3VlRu03MZz1sD668gOggBNDsKywAJyjIl2orL?=
 =?us-ascii?Q?K+XIzbxM6Op1+6LdEMsz7Kw6yxmVWPcMJgRS+FPS7ztptXFrnHtGx0Q7znpl?=
 =?us-ascii?Q?M1Yd7DU5dsVfqM7k7EjRYwNkotxmEwm7dRNzYm2/iN64vqemCpgWehwm4MIX?=
 =?us-ascii?Q?AWIPfCq9HXddd/ca4YIpJu5lG7kWfuGlvsjrNqM9Z222jMjcbSmK6t+OmGZQ?=
 =?us-ascii?Q?E7NLicH1pTYXOj2ManRhU8MQpTnaN+Os5Ukb83dTubq2pfRNiGjCern2AlMZ?=
 =?us-ascii?Q?eAq8QnoHMViSG7J9bejcCFAFY1IHb8wo6Do5Qcr0pvvxcestoPQWyG0X68Da?=
 =?us-ascii?Q?5IKBWdbH0XJw247+8BcYf3jG6RmDg2NQ+qszaz9H86uP1tdtxk6rZIFQ/Io9?=
 =?us-ascii?Q?5AhtXct+ltjzmiwjvUNDs5hybk+e4TV18gT7teSdnquls4NeYeYI3HLtdWpk?=
 =?us-ascii?Q?DpEmuSVOh4JW7uK9GUFfKPWNEjTb8vZrXzVGN50+L6pUN/eSBmqOmuoeADlN?=
 =?us-ascii?Q?Xklw+xpQVFx5TLSuYXSirAa4vAiYAGexkuVprVddPMWirlR2m5bWHFw+kXMu?=
 =?us-ascii?Q?1w8FVsPghsrQOk9eOFklRGY7Tdgt9SsPEksNvMQ4cp5h6Qe117AeWrszodFQ?=
 =?us-ascii?Q?epWOgffr+s1YWs0wICiqfAjInyLnwTTF97DuwAke9mWmMTvreAFfEfH3ZUXa?=
 =?us-ascii?Q?8cQ2MsQu47GGo9f7ZH8DFH1oA2NSTe51zS3d2o/dvkHYsJohfy0ynBQlfPtj?=
 =?us-ascii?Q?BeVcPD95rYwRdY3NihIgttomI7l+Pef3X2mqQVcRGTf1wRRCvBJaAlte8195?=
 =?us-ascii?Q?EogHZA3PtvxXjnzC+uTSeWLVWV9uwHCZ549vEYIHgevfZUUHVYY/yyVHkpzU?=
 =?us-ascii?Q?4MVn64FAFqqjnnNgbszYSxnCYRG/6Yxg8DcF8R3fdcUOju1Ze8wxCTOe8KDE?=
 =?us-ascii?Q?uOjvlpppho/A1M/6AiidUdmLL4CWLsPxp2Q4Lu1n6s9e3g8VGu8TOG+Ko6pd?=
 =?us-ascii?Q?an/FXyKor0fHvjMyodnVsg4CM70Q7HKSNfg130qyWAL8LFeI1LXNgJb1btie?=
 =?us-ascii?Q?M1liJXsNe0dRL6quEcYcW2uaZItWYKvYy15j7tnPcN/pkcMtQAC4RqkdJ5Qm?=
 =?us-ascii?Q?RdzHLdpPy9EykT7vL/9MiJOblj5j8unj1yGzeBz49nyxL/7JtrE9DyhcxwyY?=
 =?us-ascii?Q?fngP4IiElHA7zqebU8E8z4CV//63rtLAM6Br3DSZ2uPJJZW1LOuVNZc/hiz7?=
 =?us-ascii?Q?p6b6eVfwBECEyexdGfZlAUfLBCNURD1xwsiO0K7UeINHm7TW9GXyEb82cbUo?=
 =?us-ascii?Q?zr4slrbUUdrH1o9rE1pvrzOO4u3LeA95GpxfpaVZhpyw2uL5qt6QoitHBsHO?=
 =?us-ascii?Q?MYn5BuYvqxRCq2aUSTth5OcI+vLndL5lbpGsdvlH?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB8404.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d9b57fe0-e96a-4e95-6f9c-08da6642fc3c
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jul 2022 09:18:31.1884
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: J+OvSNk5Hjxi8X2nm6qFebN4GGIdl13rdi7hA1nar8ewAUwcY5PWQIzSngiihkqFr7XZfFagvbQVGwMgZknusg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB8096
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: writing a 1 and then a =
0 to
> trigger a break character
>=20
> Hi,
>=20
> 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).
>=20
> That is expected. no? There is also the TIOCCBRK which will clear the
> break. TIOCSBRK will just turn the break on.
>=20
> 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.
>=20

Hi Michael,

Seems we have the different understanding of the break_ctl(port,ctl) callba=
ck. 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 u=
nderstanding.=20

And my understanding of break_ctl(tty,0) is that it will terminate the brea=
k 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.
>=20
> 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.)
>=20
> > 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.
>=20
> -michael