| Message ID | 20201207165416.7279-1-vijai@behindbytes.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | lib: utils: Fix shakti uart implementation | expand |
> On 7 Dec 2020, at 16:54, Vijai Kumar K <vijai@behindbytes.com> wrote: > > Fix uart_putc implementation. > Due to a bug in the IP, this went unnoticed. > Use macros instead of magic numbers to make the code > more readable. Yeah, using macros is good, but this patch isn't doing anything other than adding macros; there's no change of behaviour? Either you've missed something or your commit message needs to be fixed. Also see below for a style comment. Jess > Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> > --- > lib/utils/serial/shakti-uart.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > index 493edcf..b2deb70 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -18,18 +18,21 @@ > #define REG_IQ_CYCLES 0x1C > #define REG_RX_THRES 0x20 > > +#define UART_TX_FULL 0x2 > +#define UART_RX_FULL 0x8 > + > static volatile void *uart_base; > > void shakti_uart_putc(char ch) > { > - while((readw(uart_base + REG_STATUS) & 0x2) == 0); > + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); You should have a space after the while. Also, the existing style is to put the semicolon on a new line so it's not accidentally overlooked. > writeb(ch, uart_base + REG_TX); > } > > int shakti_uart_getc(void) > { > u16 status = readw(uart_base + REG_STATUS); > - if (status & 0x8) > + if (status & UART_RX_FULL) > return readb(uart_base + REG_RX); > return -1; > } > -- > 2.25.1
Hi Jessica, Thank you for reviewing the code. Please see the inline responses. ---- On Mon, 07 Dec 2020 22:44:06 +0530 Jessica Clarke <jrtc27@jrtc27.com> wrote ---- > > On 7 Dec 2020, at 16:54, Vijai Kumar K <vijai@behindbytes.com> wrote: > > > > Fix uart_putc implementation. > > Due to a bug in the IP, this went unnoticed. > > Use macros instead of magic numbers to make the code > > more readable. > > Yeah, using macros is good, but this patch isn't doing anything other > than adding macros; there's no change of behaviour? Either you've > missed something or your commit message needs to be fixed. If you see we are dropping the "==0" comparison. That UART_TX_FULL bit is set when TX is full. So that comparison is wrong and we are dropping it. > > Also see below for a style comment. > > Jess > > > Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> > > --- > > lib/utils/serial/shakti-uart.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > index 493edcf..b2deb70 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -18,18 +18,21 @@ > > #define REG_IQ_CYCLES 0x1C > > #define REG_RX_THRES 0x20 > > > > +#define UART_TX_FULL 0x2 > > +#define UART_RX_FULL 0x8 > > + > > static volatile void *uart_base; > > > > void shakti_uart_putc(char ch) > > { > > - while((readw(uart_base + REG_STATUS) & 0x2) == 0); > > + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); > > You should have a space after the while. Also, the existing style is to > put the semicolon on a new line so it's not accidentally overlooked. Got it. Will add the space. However, I don't see the style recommendation for putting the semicolon in a new line. Have I overlooked it, or is it just an undocumented practice or a personal taste? Thanks Vijai > > > writeb(ch, uart_base + REG_TX); > > } > > > > int shakti_uart_getc(void) > > { > > u16 status = readw(uart_base + REG_STATUS); > > - if (status & 0x8) > > + if (status & UART_RX_FULL) > > return readb(uart_base + REG_RX); > > return -1; > > } > > -- > > 2.25.1 > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi >
On Tue, Dec 8, 2020 at 9:38 AM Vijai Kumar K <vijai@behindbytes.com> wrote: > > Hi Jessica, > > Thank you for reviewing the code. Please see the inline responses. > > > ---- On Mon, 07 Dec 2020 22:44:06 +0530 Jessica Clarke <jrtc27@jrtc27.com> wrote ---- > > > > On 7 Dec 2020, at 16:54, Vijai Kumar K <vijai@behindbytes.com> wrote: > > > > > > Fix uart_putc implementation. > > > Due to a bug in the IP, this went unnoticed. > > > Use macros instead of magic numbers to make the code > > > more readable. > > > > Yeah, using macros is good, but this patch isn't doing anything other > > than adding macros; there's no change of behaviour? Either you've > > missed something or your commit message needs to be fixed. > > If you see we are dropping the "==0" comparison. That UART_TX_FULL > bit is set when TX is full. So that comparison is wrong and we are dropping > it. > > > > > Also see below for a style comment. > > > > Jess > > > > > Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> > > > --- > > > lib/utils/serial/shakti-uart.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c > > > index 493edcf..b2deb70 100644 > > > --- a/lib/utils/serial/shakti-uart.c > > > +++ b/lib/utils/serial/shakti-uart.c > > > @@ -18,18 +18,21 @@ > > > #define REG_IQ_CYCLES 0x1C > > > #define REG_RX_THRES 0x20 > > > > > > +#define UART_TX_FULL 0x2 > > > +#define UART_RX_FULL 0x8 > > > + > > > static volatile void *uart_base; > > > > > > void shakti_uart_putc(char ch) > > > { > > > - while((readw(uart_base + REG_STATUS) & 0x2) == 0); > > > + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); > > > > You should have a space after the while. Also, the existing style is to > > put the semicolon on a new line so it's not accidentally overlooked. > > Got it. Will add the space. However, I don't see the style recommendation > for putting the semicolon in a new line. Have I overlooked it, or is it > just an undocumented practice or a personal taste? Yes, style documentation is not available yet. We are largely following Linux and U-Boot coding style. I will wait for your revised patch. Regards, Anup
> -----Original Message----- > From: opensbi <opensbi-bounces@lists.infradead.org> On Behalf Of Vijai > Kumar K > Sent: 07 December 2020 22:24 > To: opensbi@lists.infradead.org > Cc: Vijai Kumar K <vijai@behindbytes.com> > Subject: [PATCH] lib: utils: Fix shakti uart implementation > > Fix uart_putc implementation. > Due to a bug in the IP, this went unnoticed. > Use macros instead of magic numbers to make the code more readable. > > Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> > --- > lib/utils/serial/shakti-uart.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c index > 493edcf..b2deb70 100644 > --- a/lib/utils/serial/shakti-uart.c > +++ b/lib/utils/serial/shakti-uart.c > @@ -18,18 +18,21 @@ > #define REG_IQ_CYCLES 0x1C > #define REG_RX_THRES 0x20 > > +#define UART_TX_FULL 0x2 > +#define UART_RX_FULL 0x8 > + > static volatile void *uart_base; > > void shakti_uart_putc(char ch) > { > - while((readw(uart_base + REG_STATUS) & 0x2) == 0); > + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); > writeb(ch, uart_base + REG_TX); > } > > int shakti_uart_getc(void) > { > u16 status = readw(uart_base + REG_STATUS); > - if (status & 0x8) > + if (status & UART_RX_FULL) > return readb(uart_base + REG_RX); > return -1; > } > -- > 2.25.1 > > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi Reviewed-by: Anup Patel <anup.patel@wdc.com> I have taken care of style comment from Jessica at time of applying this patch. Applied this patch to riscv/opensbi repo. Thanks, Anup
---- On Thu, 07 Jan 2021 10:27:17 +0530 Anup Patel <Anup.Patel@wdc.com> wrote ---- > > > > -----Original Message----- > > From: opensbi <opensbi-bounces@lists.infradead.org> On Behalf Of Vijai > > Kumar K > > Sent: 07 December 2020 22:24 > > To: opensbi@lists.infradead.org > > Cc: Vijai Kumar K <vijai@behindbytes.com> > > Subject: [PATCH] lib: utils: Fix shakti uart implementation > > > > Fix uart_putc implementation. > > Due to a bug in the IP, this went unnoticed. > > Use macros instead of magic numbers to make the code more readable. > > > > Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> > > --- > > lib/utils/serial/shakti-uart.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c index > > 493edcf..b2deb70 100644 > > --- a/lib/utils/serial/shakti-uart.c > > +++ b/lib/utils/serial/shakti-uart.c > > @@ -18,18 +18,21 @@ > > #define REG_IQ_CYCLES 0x1C > > #define REG_RX_THRES 0x20 > > > > +#define UART_TX_FULL 0x2 > > +#define UART_RX_FULL 0x8 > > + > > static volatile void *uart_base; > > > > void shakti_uart_putc(char ch) > > { > > - while((readw(uart_base + REG_STATUS) & 0x2) == 0); > > + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); > > writeb(ch, uart_base + REG_TX); > > } > > > > int shakti_uart_getc(void) > > { > > u16 status = readw(uart_base + REG_STATUS); > > - if (status & 0x8) > > + if (status & UART_RX_FULL) > > return readb(uart_base + REG_RX); > > return -1; > > } > > -- > > 2.25.1 > > > > > > > > -- > > opensbi mailing list > > opensbi@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/opensbi > > Reviewed-by: Anup Patel <anup.patel@wdc.com> > > I have taken care of style comment from Jessica at time of > applying this patch. > > Applied this patch to riscv/opensbi repo. > > Thanks, > Anup Thanks a lot Anup. Sorry for replying late on this. I was on vacation at that time and somehow missed following up on this. Best, Vijai > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi >
diff --git a/lib/utils/serial/shakti-uart.c b/lib/utils/serial/shakti-uart.c index 493edcf..b2deb70 100644 --- a/lib/utils/serial/shakti-uart.c +++ b/lib/utils/serial/shakti-uart.c @@ -18,18 +18,21 @@ #define REG_IQ_CYCLES 0x1C #define REG_RX_THRES 0x20 +#define UART_TX_FULL 0x2 +#define UART_RX_FULL 0x8 + static volatile void *uart_base; void shakti_uart_putc(char ch) { - while((readw(uart_base + REG_STATUS) & 0x2) == 0); + while((readw(uart_base + REG_STATUS) & UART_TX_FULL)); writeb(ch, uart_base + REG_TX); } int shakti_uart_getc(void) { u16 status = readw(uart_base + REG_STATUS); - if (status & 0x8) + if (status & UART_RX_FULL) return readb(uart_base + REG_RX); return -1; }
Fix uart_putc implementation. Due to a bug in the IP, this went unnoticed. Use macros instead of magic numbers to make the code more readable. Signed-off-by: Vijai Kumar K <vijai@behindbytes.com> --- lib/utils/serial/shakti-uart.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)