On 4/3/2018 1:15 PM, Evan Green wrote:
> Hi Karthik,
>
> On Fri, Mar 23, 2018 at 3:10 PM Karthikeyan Ramasubramanian <
> kramasub@xxxxxxxxxxxxxx> wrote:
>
>> The driver has some follow-up comments right after it got merged. This
>> patch addresses those comments that got missed out.
>
>> * Document reason for newline character counting in console_write
>> * Document reason for disabling IRQ in the system resume operation
>> * Use min3 to find the minimum of 3 values
>> * Remove unnecessary casting while using min_t
>> * Use iowrite32_rep to write to the hardware FIFO
>> * Initialize the console port statically
>> * Fine-tune memory barrier usage
>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@xxxxxxxxxxxxxx>
>> ---
>> drivers/tty/serial/qcom_geni_serial.c | 84
> ++++++++++++++++++-----------------
>> 1 file changed, 44 insertions(+), 40 deletions(-)
>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
>> index 1442777..f5b9cb8 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
> ...
>> @@ -406,20 +421,18 @@ static void qcom_geni_serial_start_tx(struct
> uart_port *uport)
>> u32 status;
>
>> if (port->xfer_mode == GENI_SE_FIFO) {
>> - status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> + /*
>> + * readl ensures reading & writing of IRQ_EN register
>> + * is not re-ordered before checking the status of the
>> + * Serial Engine.
>> + */
>> + status = readl(uport->membase + SE_GENI_STATUS);
>> if (status & M_GENI_CMD_ACTIVE)
>> return;
>
>> if (!qcom_geni_serial_tx_empty(uport))
>> return;
>
>> - /*
>> - * Ensure writing to IRQ_EN & watermark registers are not
>> - * re-ordered before checking the status of the Serial
>> - * Engine and TX FIFO
>> - */
>> - mb();
>> -
>> irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
>
> Thanks for doing this fixup in start_tx. The same pattern would apply in
> stop_tx, start_rx, and stop_rx, right?
The read barrier coupled with the data dependency ensures that the write
to IRQ_EN is not re-ordered ahead of checking the GENI_STATUS. The write
to watermark register can still be re-ordered but it does not cause any
impact.
In the other cases, the cancel command is written to a different
register. There is no data dependency and hence if cancel command write
gets reordered ahead of checking the GENI_STATUS, then the code may
return prematurely before some clean-up is done.
>
> -Evan
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
