Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yash Shah <yash.shah@sifive.com>
To:     Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>
CC:     Palmer Dabbelt <palmerdabbelt@google.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        "jslaby@suse.com" <jslaby@suse.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
        "kernel-team@android.com" <kernel-team@android.com>,
        Sachin Ghadi <sachin.ghadi@sifive.com>
Subject: RE: [PATCH] tty: sifive: Finish transmission before changing the
 clock
Thread-Topic: [PATCH] tty: sifive: Finish transmission before changing the
 clock
Thread-Index: AQHV9DjmyxKWiUuFkUuyXFb+1DKKS6hDUOug
Date:   Wed, 11 Mar 2020 12:22:04 +0000
Message-ID: <MN2PR13MB3552C9E76C003A8A302540808CFC0@MN2PR13MB3552.namprd13.prod.outlook.com>
References: <20200307042637.83728-1-palmer@dabbelt.com>
In-Reply-To: <20200307042637.83728-1-palmer@dabbelt.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: sifive.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 364698b5-4faa-47d1-6285-08d7c5b6cf0c
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Mar 2020 12:22:04.1671
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 22f88e9d-ae0d-4ed9-b984-cdc9be1529f1
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: cagPcr2urqiWRqKNuuwhp/sm6z5rtW7KJtRhgCaAjKloLJbJXYYSXcfotfgWHD7RguwRHok/kHL4rz208bP4Jw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR13MB2734
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: linux-riscv <linux-riscv-bounces@lists.infradead.org> On Behalf Of
> Palmer Dabbelt
> Sent: 07 March 2020 09:57
> To: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmerdabbelt@google.com>; Greg KH
> <gregkh@linuxfoundation.org>; jslaby@suse.com; linux-
> kernel@vger.kernel.org; Palmer Dabbelt <palmer@dabbelt.com>; linux-
> serial@vger.kernel.org; Paul Walmsley <paul.walmsley@sifive.com>; linux-
> riscv@lists.infradead.org; kernel-team@android.com
> Subject: [PATCH] tty: sifive: Finish transmission before changing the clo=
ck
>=20
> From: Palmer Dabbelt <palmerdabbelt@google.com>
>=20
> SiFive's UART has a software controller clock divider that produces the f=
inal
> baud rate clock.  Whenever the clock that drives the UART is changed this
> divider must be updated accordingly, and given that these two events are
> controlled by software they cannot be done atomically.
> During the period between updating the UART's driving clock and internal
> divider the UART will transmit a different baud rate than what the user h=
as
> configured, which will probably result in a corrupted transmission stream=
.
>=20
> The SiFive UART has a FIFO, but due to an issue with the programming
> interface there is no way to directly determine when the UART has finishe=
d
> transmitting.  We're essentially restricted to dead reckoning in order to=
 figure
> that out: we can use the FIFO's TX busy register to figure out when the l=
ast
> frame has begun transmission and just delay for a long enough that the la=
st
> frame is guaranteed to get out.
>=20
> As far as the actual implementation goes: I've modified the existing exis=
ting
> clock notifier function to drain both the FIFO and the shift register in =
on
> PRE_RATE_CHANGE.  As far as I know there is no hardware flow control in
> this UART, so there's no good way to ask the other end to stop transmissi=
on
> while we can't receive (inserting software flow control messages seems li=
ke a
> bad idea here).
>=20
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> ---
> I have not tested this, as I don't have any hardware.  I'm also not even
> remotely familiar with the serial subsystem, so I don't know if there's a
> better way of going about this.  I'm specifically worried about a udelay(=
) that
> could be quite long.  Maybe some sort of "delay for short times, sleep fo=
r
> long times" approach would be better?
>=20
> I don't know if this manifests in practice on existing hardware when runn=
ing
> real workloads, but I'd be willing to bet that it would be possible to tr=
igger
> the bug on purpose as by my calculations there's about a 10k cycle window=
 in
> which the clock can't change.  IIRC there's a lot of instability when cha=
nging
> the clock frequency on the HiFive Unleashed so I doubt people are going t=
o
> stumble across the issue regularly in practice.
>=20
>  drivers/tty/serial/sifive.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c in=
dex
> d5f81b98e4d7..d34031e842d0 100644
> --- a/drivers/tty/serial/sifive.c
> +++ b/drivers/tty/serial/sifive.c
> @@ -618,10 +618,10 @@ static void sifive_serial_shutdown(struct uart_port
> *port)
>   *
>   * On the V0 SoC, the UART IP block is derived from the CPU clock source
>   * after a synchronous divide-by-two divider, so any CPU clock rate chan=
ge
> - * requires the UART baud rate to be updated.  This presumably could
> corrupt any
> - * serial word currently being transmitted or received.  It would probab=
ly
> - * be better to stop receives and transmits, then complete the baud rate
> - * change, then re-enable them.
> + * requires the UART baud rate to be updated.  This presumably corrupts
> + any
> + * serial word currently being transmitted or received.  In order to
> + avoid
> + * corrupting the output data stream, we drain the transmit queue
> + before
> + * allowing the clock's rate to be changed.
>   */
>  static int sifive_serial_clk_notifier(struct notifier_block *nb,
>  				      unsigned long event, void *data) @@ -
> 629,6 +629,26 @@ static int sifive_serial_clk_notifier(struct notifier_bl=
ock
> *nb,
>  	struct clk_notifier_data *cnd =3D data;
>  	struct sifive_serial_port *ssp =3D notifier_to_sifive_serial_port(nb);
>=20
> +	if (event =3D=3D PRE_RATE_CHANGE) {
> +		/*
> +		 * The TX watermark is always set to 1 by this driver, which
> +		 * means that the TX busy bit will lower when there are 0
> bytes
> +		 * left in the TX queue -- in other words, when the TX FIFO is
> +		 * empty.
> +		 */
> +		__ssp_wait_for_xmitr(ssp);
> +		/*
> +		 * On the cycle the TX FIFO goes empty there is still a full
> +		 * UART frame left to be transmitted in the shift register.
> +		 * The UART provides no way for software to directly
> determine
> +		 * when that last frame has been transmitted, so we just
> sleep
> +		 * here instead.  As we're not tracking the number of stop
> bits
> +		 * they're just worst cased here.  The rest of the serial
> +		 * framing parameters aren't configurable by software.
> +		 */
> +		udelay(DIV_ROUND_UP(12 * 1000 * 1000, ssp->baud_rate));
> +	}
> +
>  	if (event =3D=3D POST_RATE_CHANGE && ssp->clkin_rate !=3D cnd-
> >new_rate) {
>  		ssp->clkin_rate =3D cnd->new_rate;
>  		__ssp_update_div(ssp);
> --
> 2.25.1.481.gfbce0eb801-goog
>=20

A quick test on HiFive Unleashed board showed some improvements.
Prior to this patch, I have been observing some random corrupted characters=
 on serial console when continuously changing the CPU clock rate.
After applying this patch I don't see those corrupted characters anymore wh=
ile changing the clock rate.

Tested-by: Yash Shah <yash.shah@sifive.com>

This observation is based on a quick initial test on HiFive Unleashed. I am=
 planning to further test it by inducing the error on purpose. Will try to =
update the result soon.

- Yash