| Message ID | 1277830727-1794-1-git-send-email-brian@cozybit.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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
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.
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
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.
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();
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(-)