[PATCH v3] zynq-uart: Fix set_attributes implementation

[Gedare Bloom]

On Thu, Dec 3, 2020 at 9:46 AM Gedare Bloom <gedare at rtems.org> wrote:

>
>
> On Thu, Dec 3, 2020 at 8:25 AM Kinsey Moore <kinsey.moore at oarcorp.com>
> wrote:
>
>> The zynq-uart set_attributes implementation was configured to always
>> return false which causes spconsole01 to fail. This restores the
>> disabled implementation which sets the baud rate registers
>> appropriately and allows spconsole01 to pass. This also expands the
>> set_attributes functionality to allow setting of the stop bits,
>> character width, and parity.
>> ---
>>  bsps/include/dev/serial/zynq-uart.h       |  7 +++
>>  bsps/shared/dev/serial/zynq-uart-polled.c |  2 +-
>>  bsps/shared/dev/serial/zynq-uart.c        | 64 +++++++++++++++++++++--
>>  3 files changed, 67 insertions(+), 6 deletions(-)
>>
>> diff --git a/bsps/include/dev/serial/zynq-uart.h
>> b/bsps/include/dev/serial/zynq-uart.h
>> index 2c0f250a3a..0eb1dd5f29 100644
>> --- a/bsps/include/dev/serial/zynq-uart.h
>> +++ b/bsps/include/dev/serial/zynq-uart.h
>> @@ -78,6 +78,13 @@ void zynq_uart_write_polled(
>>    */
>>  void zynq_uart_reset_tx_flush(zynq_uart_context *ctx);
>>
>> +int zynq_cal_baud_rate(
>> +  uint32_t  baudrate,
>> +  uint32_t* brgr,
>> +  uint32_t* bauddiv,
>> +  uint32_t  modereg
>> +);
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif /* __cplusplus */
>> diff --git a/bsps/shared/dev/serial/zynq-uart-polled.c
>> b/bsps/shared/dev/serial/zynq-uart-polled.c
>> index a1b51ea521..442431d502 100644
>> --- a/bsps/shared/dev/serial/zynq-uart-polled.c
>> +++ b/bsps/shared/dev/serial/zynq-uart-polled.c
>> @@ -40,7 +40,7 @@ uint32_t zynq_uart_input_clock(void)
>>    return ZYNQ_CLOCK_UART;
>>  }
>>
>> -static int zynq_cal_baud_rate(uint32_t  baudrate,
>> +int zynq_cal_baud_rate(uint32_t  baudrate,
>>                                uint32_t* brgr,
>>                                uint32_t* bauddiv,
>>                                uint32_t  modereg)
>> diff --git a/bsps/shared/dev/serial/zynq-uart.c
>> b/bsps/shared/dev/serial/zynq-uart.c
>> index 41adb196ab..39e2e65924 100644
>> --- a/bsps/shared/dev/serial/zynq-uart.c
>> +++ b/bsps/shared/dev/serial/zynq-uart.c
>> @@ -142,25 +142,79 @@ static bool zynq_uart_set_attributes(
>>    const struct termios *term
>>  )
>>  {
>> -#if 0
>> -  volatile zynq_uart *regs = zynq_uart_get_regs(minor);
>> +  zynq_uart_context *ctx = (zynq_uart_context *) context;
>> +  volatile zynq_uart *regs = ctx->regs;
>>    uint32_t brgr = 0;
>>    uint32_t bauddiv = 0;
>> +  uint32_t mode = 0;
>>    int rc;
>>
>>    rc = zynq_cal_baud_rate(115200, &brgr, &bauddiv, regs->mode);
>>    if (rc != 0)
>>      return rc;
>>
>> +  /*
>> +   * Configure the mode register
>> +   */
>> +  mode |= ZYNQ_UART_MODE_CHMODE(ZYNQ_UART_MODE_CHMODE_NORMAL);
>> +
>> +  /*
>> +   * Parity
>> +   */
>> +
>>
> I'd avoid extra vertical space between comments and relevant code. The
> Parity comment applies to the following block of code, so it should be kept
> "closer" to it for readability. (This may not be an official style rule,
> but seems good practice.)
>
>> +  mode |= ZYNQ_UART_MODE_PAR(ZYNQ_UART_MODE_PAR_NONE);
>> +  if (term->c_cflag & PARENB) {
>> +    if (!(term->c_cflag & PARODD)) {
>> +      mode |= ZYNQ_UART_MODE_PAR(ZYNQ_UART_MODE_PAR_ODD);
>> +    } else {
>> +      mode |= ZYNQ_UART_MODE_PAR(ZYNQ_UART_MODE_PAR_EVEN);
>> +    }
>> +  }
>> +
>> +  /*
>> +   * Character Size
>> +   */
>> +
>> +  if (term->c_cflag & CSIZE) {
>> +    switch (term->c_cflag & CSIZE)
>>
> Duplicating the conditional isn't needed. Let the switch handle the
> default case also, like:
>
>
>> +    {
>> +    case CS6:
>> +      mode |= ZYNQ_UART_MODE_CHRL(ZYNQ_UART_MODE_CHRL_6);
>> +      break;
>> +    case CS7:
>> +      mode |= ZYNQ_UART_MODE_CHRL(ZYNQ_UART_MODE_CHRL_7);
>> +      break;
>> +    case CS8:
>>
>           case 0:
>
>> +      mode |= ZYNQ_UART_MODE_CHRL(ZYNQ_UART_MODE_CHRL_8);
>> +      break;
>
> +    default:
>> +      return false;
>>
> One more thing, I think this default case is dead code and can be removed.


> +    }
>> +  } else {
>> +    /* default to 9600,8,N,1 */
>> +    mode |= ZYNQ_UART_MODE_CHRL(ZYNQ_UART_MODE_CHRL_8);
>> +  }
>> +
>> +  /*
>> +   * Stop Bits
>> +   */
>> +
>> +  if (term->c_cflag & CSTOPB) {
>> +    /* 2 stop bits */
>> +    mode |= ZYNQ_UART_MODE_NBSTOP(ZYNQ_UART_MODE_NBSTOP_STOP_2);
>> +  } else {
>> +    /* 1 stop bit */
>> +    mode |= ZYNQ_UART_MODE_NBSTOP(ZYNQ_UART_MODE_NBSTOP_STOP_1);
>> +  }
>> +
>> +
>>
> avoid 2+ blank lines in a row
>
>
>>    regs->control &= ~(ZYNQ_UART_CONTROL_RXEN | ZYNQ_UART_CONTROL_TXEN);
>> +  regs->mode = mode;
>>    regs->baud_rate_gen = ZYNQ_UART_BAUD_RATE_GEN_CD(brgr);
>>    regs->baud_rate_div = ZYNQ_UART_BAUD_RATE_DIV_BDIV(bauddiv);
>>    regs->control |= ZYNQ_UART_CONTROL_RXEN | ZYNQ_UART_CONTROL_TXEN;
>>
>>    return true;
>> -#else
>> -  return false;
>> -#endif
>>  }
>>
>>  const rtems_termios_device_handler zynq_uart_handler = {
>> --
>> 2.20.1
>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20201203/743e288f/attachment.html>