Patchwork [U-Boot] bug fix and cleanup for s3c44b0 serial driver

login
register
mail settings
Submitter Brian Cavagnolo
Date June 29, 2010, 4:58 p.m.
Message ID <1277830727-1794-1-git-send-email-brian@cozybit.com>
Download mbox | patch
Permalink /patch/71794/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Brian Cavagnolo - June 29, 2010, 4:58 p.m.
Specifically, don't dereference the URXH0 register twice; calculate the BRD
based on the formula in the databook instead of using a messy switch statement;
and migrate the BRD calculation to the hardware hardware header because it's
board specific.

Signed-off-by: Brian Cavagnolo <brian@cozybit.com>
---
 arch/arm/include/asm/arch-s3c44b0/hardware.h |    3 +-
 drivers/serial/serial_s3c44b0.c              |   65 +-------------------------
 2 files changed, 4 insertions(+), 64 deletions(-)
Wolfgang Denk - Aug. 8, 2010, 8:23 p.m.
Dear Minkyu Kang,

In message <1277830727-1794-1-git-send-email-brian@cozybit.com> Brian Cavagnolo wrote:
> Specifically, don't dereference the URXH0 register twice; calculate the BRD
> based on the formula in the databook instead of using a messy switch statement;
> and migrate the BRD calculation to the hardware hardware header because it's
> board specific.
> 
> Signed-off-by: Brian Cavagnolo <brian@cozybit.com>
> ---
>  arch/arm/include/asm/arch-s3c44b0/hardware.h |    3 +-
>  drivers/serial/serial_s3c44b0.c              |   65 +-------------------------
>  2 files changed, 4 insertions(+), 64 deletions(-)

Is this patch on your queue?  See
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/80548

Best regards,

Wolfgang Denk
Minkyu Kang - Aug. 10, 2010, 5:48 a.m.
Dear Brian Cavagnolo,

On 30 June 2010 01:58, Brian Cavagnolo <brian@cozybit.com> wrote:
> Specifically, don't dereference the URXH0 register twice; calculate the BRD
> based on the formula in the databook instead of using a messy switch statement;
> and migrate the BRD calculation to the hardware hardware header because it's

hardware hardware..? is it typo?

> board specific.
>
> Signed-off-by: Brian Cavagnolo <brian@cozybit.com>
> ---
>  arch/arm/include/asm/arch-s3c44b0/hardware.h |    3 +-
>  drivers/serial/serial_s3c44b0.c              |   65 +-------------------------
>  2 files changed, 4 insertions(+), 64 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-s3c44b0/hardware.h b/arch/arm/include/asm/arch-s3c44b0/hardware.h
> index 146e265..38ff32c 100644
> --- a/arch/arm/include/asm/arch-s3c44b0/hardware.h
> +++ b/arch/arm/include/asm/arch-s3c44b0/hardware.h
> @@ -11,7 +11,8 @@
>  #define REGL(addr)     (*(volatile unsigned int *)(REGBASE+addr))
>  #define REGW(addr)     (*(volatile unsigned short *)(REGBASE+addr))
>  #define REGB(addr)     (*(volatile unsigned char *)(REGBASE+addr))
> -
> +#define BRD(bps)       (DIV_ROUND(CONFIG_S3C44B0_CLOCK_SPEED * 1000000, \
> +                               (bps)*16) - 1)

no need brace and please add the space around the operator.
bps * 16) - 1)

>
>  /*****************************/
>  /* CPU Wrapper Registers     */
> diff --git a/drivers/serial/serial_s3c44b0.c b/drivers/serial/serial_s3c44b0.c
> index 95d0266..e6c535c 100644
> --- a/drivers/serial/serial_s3c44b0.c
> +++ b/drivers/serial/serial_s3c44b0.c
> @@ -48,7 +48,7 @@ static int serial_flush_input(void)
>
>        /* keep on reading as long as the receiver is not empty */
>        while(UTRSTAT0&0x01) {

Space required before the open parenthesis '('.
Could you please run checkpatch and make clean-up patch for this file?

> -               tmp = REGB(URXH0);
> +               tmp = URXH0;
>        }
>
>        return 0;
> @@ -70,68 +70,7 @@ static int serial_flush_output(void)
>
>  void serial_setbrg (void)
>  {
> -       u32 divisor = 0;
> -
> -       /* get correct divisor */
> -       switch(gd->baudrate) {
> -
> -       case 1200:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 3124;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 3905;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif
> -               break;
> -
> -       case 9600:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 390;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 487;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif
> -               break;
> -
> -       case 19200:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 194;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 243;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif
> -               break;
> -
> -       case 38400:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 97;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 121;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif /* break; */
> -
> -       case 57600:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 64;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 80;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif /* break; */
> -
> -       case 115200:
> -#if CONFIG_S3C44B0_CLOCK_SPEED==66
> -               divisor = 32;
> -#elif CONFIG_S3C44B0_CLOCK_SPEED==75
> -               divisor = 40;
> -#else
> -# error CONFIG_S3C44B0_CLOCK_SPEED undefined
> -#endif /* break; */
> -       }
> +       u32 divisor = BRD(gd->baudrate);
>
>        serial_flush_output();
>        serial_flush_input();
> --
> 1.6.0.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>


Thanks
Minkyu Kang.
Reinhard Meyer - Aug. 10, 2010, 6:10 a.m.
Dear Minkyu Kang,

my five cents:

>>  #define REGL(addr)     (*(volatile unsigned int *)(REGBASE+addr))
>>  #define REGW(addr)     (*(volatile unsigned short *)(REGBASE+addr))
>>  #define REGB(addr)     (*(volatile unsigned char *)(REGBASE+addr))

isn't that way of accessing hardware VERY depreciated?

 >> +#define BRD(bps)       (DIV_ROUND(CONFIG_S3C44B0_CLOCK_SPEED * 1000000, \
 >> > +                               (bps)*16) - 1)
 >
 >no need brace and please add the space around the operator.
 >bps * 16) - 1)

its always wise to () a macro parameter and not make assumptions on the
parameters. What about BRD(9600+9600) ?

>>  /*****************************/
>>  /* CPU Wrapper Registers     */

I think that version of multi-line comment is not allowed...

Best Regards, Reinhard
Minkyu Kang - Aug. 10, 2010, 7:59 a.m.
Dear Reinhard Meyer,

On 10 August 2010 15:10, Reinhard Meyer <u-boot@emk-elektronik.de> wrote:
> Dear Minkyu Kang,
>
> my five cents:
>
>>>  #define REGL(addr)     (*(volatile unsigned int *)(REGBASE+addr))
>>>  #define REGW(addr)     (*(volatile unsigned short *)(REGBASE+addr))
>>>  #define REGB(addr)     (*(volatile unsigned char *)(REGBASE+addr))
>
> isn't that way of accessing hardware VERY depreciated?

Agreed, we need update for s3c44b0.
But, this patch is not for it.
I think, It would be another work.

>
>>> +#define BRD(bps)       (DIV_ROUND(CONFIG_S3C44B0_CLOCK_SPEED * 1000000,
>>> \
>>> > +                               (bps)*16) - 1)
>>
>>no need brace and please add the space around the operator.
>>bps * 16) - 1)
>
> its always wise to () a macro parameter and not make assumptions on the
> parameters. What about BRD(9600+9600) ?

Yes, you're right!
Thanks (:

>
>>>  /*****************************/
>>>  /* CPU Wrapper Registers     */
>
> I think that version of multi-line comment is not allowed...

Yes, so I requested clean-up patch for this file.

>
> Best Regards, Reinhard
>

Thanks
Minkyu Kang.

Patch

diff --git a/arch/arm/include/asm/arch-s3c44b0/hardware.h b/arch/arm/include/asm/arch-s3c44b0/hardware.h
index 146e265..38ff32c 100644
--- a/arch/arm/include/asm/arch-s3c44b0/hardware.h
+++ b/arch/arm/include/asm/arch-s3c44b0/hardware.h
@@ -11,7 +11,8 @@ 
 #define REGL(addr)	(*(volatile unsigned int *)(REGBASE+addr))
 #define REGW(addr)	(*(volatile unsigned short *)(REGBASE+addr))
 #define REGB(addr)	(*(volatile unsigned char *)(REGBASE+addr))
-
+#define BRD(bps)	(DIV_ROUND(CONFIG_S3C44B0_CLOCK_SPEED * 1000000, \
+				(bps)*16) - 1)
 
 /*****************************/
 /* CPU Wrapper Registers     */
diff --git a/drivers/serial/serial_s3c44b0.c b/drivers/serial/serial_s3c44b0.c
index 95d0266..e6c535c 100644
--- a/drivers/serial/serial_s3c44b0.c
+++ b/drivers/serial/serial_s3c44b0.c
@@ -48,7 +48,7 @@  static int serial_flush_input(void)
 
 	/* keep on reading as long as the receiver is not empty */
 	while(UTRSTAT0&0x01) {
-		tmp = REGB(URXH0);
+		tmp = URXH0;
 	}
 
 	return 0;
@@ -70,68 +70,7 @@  static int serial_flush_output(void)
 
 void serial_setbrg (void)
 {
-	u32 divisor = 0;
-
-	/* get correct divisor */
-	switch(gd->baudrate) {
-
-	case 1200:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 3124;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 3905;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif
-		break;
-
-	case 9600:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 390;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 487;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif
-		break;
-
-	case 19200:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 194;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 243;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif
-		break;
-
-	case 38400:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 97;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 121;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif	/* break; */
-
-	case 57600:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 64;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 80;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif	/* break; */
-
-	case 115200:
-#if CONFIG_S3C44B0_CLOCK_SPEED==66
-		divisor = 32;
-#elif CONFIG_S3C44B0_CLOCK_SPEED==75
-		divisor = 40;
-#else
-# error CONFIG_S3C44B0_CLOCK_SPEED undefined
-#endif	/* break; */
-	}
+	u32 divisor = BRD(gd->baudrate);
 
 	serial_flush_output();
 	serial_flush_input();