qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] SH7750/51: add register BCR3, NCR4, PCR, RTCOR


From: Shin-ichiro KAWASAKI
Subject: Re: [Qemu-devel] [PATCH] SH7750/51: add register BCR3, NCR4, PCR, RTCOR, RTCNT, RTCSR, SDMR2, SDMR3 and fix BCR2 support
Date: Sun, 14 Dec 2008 20:37:34 +0900
User-agent: Thunderbird 2.0.0.18 (Windows/20081105)

Hi, Jean-san.

Thanks for your work.  Please find my comments on the patch, in line.

# When I send mail to your address from my PC,
# mail delivery error happens as follows.
# 
#       ----- Transcript of session follows -----
#   451 4.4.1 reply: read error from mx1.ovh.net.
#   451 4.4.1 reply: read error from mx2.ovh.net.
#   <your mail address >... Deferred: Connection timed out with mx2.ovh.net.
#
# How can I keep in touch with you?

Regards,
Shin-ichiro KAWASAKI


Jean-Christophe PLAGNIOL-VILLARD wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <address@hidden>
> Cc: Takashi Yoshii <address@hidden>
> Cc: Nobuhiro Iwamatsu <address@hidden>
> ---
>  hw/sh7750.c          |   59 +++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/sh7750_regnames.c |    2 +
>  hw/sh7750_regs.h     |   11 +++++++++
>  3 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/sh7750.c b/hw/sh7750.c
> index 4ae90b1..78843f3 100644
> --- a/hw/sh7750.c
> +++ b/hw/sh7750.c
> @@ -42,8 +42,12 @@ typedef struct SH7750State {
>      uint32_t periph_freq;
>      /* SDRAM controller */
>      uint32_t bcr1;
> -    uint32_t bcr2;
> +    uint16_t bcr2;
> +    uint16_t bcr3;
> +    uint32_t bcr4;
>      uint16_t rfcr;
> +    /* PCMCIA controller */
> +    uint16_t pcr;
>      /* IO ports */
>      uint16_t gpioic;
>      uint32_t pctra;
> @@ -67,7 +71,10 @@ typedef struct SH7750State {
>      struct intc_desc intc;
>  } SH7750State;
>  
> -
> +static int inline is_sh7751r_cpu(SH7750State * s)
> +{
> +     return (s->cpu->id & (SH_CPU_SH7751R));
> +}
>  /**********************************************************************
>   I/O ports
>  **********************************************************************/
> @@ -212,8 +219,17 @@ static uint32_t sh7750_mem_readw(void *opaque, 
> target_phys_addr_t addr)
>      switch (addr) {
>      case SH7750_BCR2_A7:
>       return s->bcr2;
> +    case SH7750_BCR3_A7:
> +     if(is_sh7751r_cpu(s)) {
> +         return s->bcr3;
> +     } else {
> +         error_access("word read", addr);
> +         assert(0);
> +     }

BCR3 exists not only for SH7751R, but also SH7750.
I think is_shh751r_cpu() check and error handling
should be removed to simplify the differcence.

>      case SH7750_FRQCR_A7:
>       return 0;
> +    case SH7750_PCR_A7:
> +     return s->pcr;
>      case SH7750_RFCR_A7:
>       fprintf(stderr,
>               "Read access to refresh count register, incrementing\n");
> @@ -222,6 +238,11 @@ static uint32_t sh7750_mem_readw(void *opaque, 
> target_phys_addr_t addr)
>       return porta_lines(s);
>      case SH7750_PDTRB_A7:
>       return portb_lines(s);
> +    case SH7750_RTCOR_A7:
> +    case SH7750_RTCNT_A7:
> +    case SH7750_RTCSR_A7:
> +     ignore_access("word read", addr);
> +     return 0;
>      case 0x1fd00000:
>          return s->icr;
>      default:
> @@ -238,6 +259,12 @@ static uint32_t sh7750_mem_readl(void *opaque, 
> target_phys_addr_t addr)
>      case SH7750_BCR1_A7:
>       return s->bcr1;
>      case SH7750_BCR4_A7:
> +     if(is_sh7751r_cpu(s)) {
> +         return s->bcr4;
> +     } else {
> +         error_access("long read", addr);
> +         assert(0);
> +     }

For BCR4, same as above.

>      case SH7750_WCR1_A7:
>      case SH7750_WCR2_A7:
>      case SH7750_WCR3_A7:
> @@ -274,9 +301,17 @@ static uint32_t sh7750_mem_readl(void *opaque, 
> target_phys_addr_t addr)
>      }
>  }
>  
> +#define is_in_sdrmx(a, x) (a >= SH7750_SDMR ## x ## _A7 \
> +                     && a <= (SH7750_SDMR ## x ## _A7 + SH7750_SDMR ## x ## 
> _REGNB))
>  static void sh7750_mem_writeb(void *opaque, target_phys_addr_t addr,
>                             uint32_t mem_value)
>  {
> +
> +    if (is_in_sdrmx(addr, 2) || is_in_sdrmx(addr, 3)) {
> +     ignore_access("word write", addr);
> +     return;
> +    }
> +

SRMR2 and SDMR3 region overlaps with the PRECHARGE0/1 register.
If you introduce them, PRECHARGE0/1 register should be removed.
# U-Boot does not seem to touch these regions.  Does it?

Compared to 'if' statement, 'switch-case' might be more easy to
understand, like as follows.
  case SH7750_SDMR2 ... SH7750_SDMR2 + SDMR2_REGNB

>      switch (addr) {
>       /* PRECHARGE ? XXXXX */
>      case SH7750_PRECHARGE0_A7:
> @@ -301,8 +336,18 @@ static void sh7750_mem_writew(void *opaque, 
> target_phys_addr_t addr,
>       s->bcr2 = mem_value;
>       return;
>      case SH7750_BCR3_A7:
> -    case SH7750_RTCOR_A7:
> +     if(is_sh7751r_cpu(s)) {
> +         s->bcr3 = mem_value;
> +         return;
> +     } else {
> +         error_access("word write", addr);
> +         assert(0);
> +     }

Same as readw for BCR3.

> +    case SH7750_PCR_A7:
> +     s->pcr = mem_value;
> +     return;
>      case SH7750_RTCNT_A7:
> +    case SH7750_RTCOR_A7:
>      case SH7750_RTCSR_A7:
>       ignore_access("word write", addr);
>       return;
> @@ -349,6 +394,14 @@ static void sh7750_mem_writel(void *opaque, 
> target_phys_addr_t addr,
>       s->bcr1 = mem_value;
>          return;
>      case SH7750_BCR4_A7:
> +     if(is_sh7751r_cpu(s)) {
> +         s->bcr4 = mem_value;
> +         return;
> +     } else {
> +         error_access("long write", addr);
> +         assert(0);
> +     }
> +        return;
>      case SH7750_WCR1_A7:
>      case SH7750_WCR2_A7:
>      case SH7750_WCR3_A7:
> diff --git a/hw/sh7750_regnames.c b/hw/sh7750_regnames.c
> index 51283c9..77993c1 100644
> --- a/hw/sh7750_regnames.c
> +++ b/hw/sh7750_regnames.c
> @@ -79,6 +79,8 @@ static regname_t regnames[] = {
>       REGNAME(SH7750_ICR_A7)
>       REGNAME(SH7750_BCR3_A7)
>       REGNAME(SH7750_BCR4_A7)
> +     REGNAME(SH7750_SDMR2_A7)
> +     REGNAME(SH7750_SDMR3_A7)
>       REGNAME(SH7750_PRECHARGE0_A7)
>      REGNAME(SH7750_PRECHARGE1_A7) {(uint32_t) - 1, 0}
>  };
> diff --git a/hw/sh7750_regs.h b/hw/sh7750_regs.h
> index c8fb328..4ed471b 100644
> --- a/hw/sh7750_regs.h
> +++ b/hw/sh7750_regs.h
> @@ -979,6 +979,17 @@
>  
>  #define SH7750_RFCR_KEY       0xA400 /* RFCR write key */
>  
> +/* Synchronous DRAM mode registers - SDMR */
> +#define SH7750_SDMR2_REGOFS   0x900000       /* base offset */
> +#define SH7750_SDMR2_REGNB    0x0FFC /* nb of register */
> +#define SH7750_SDMR2          SH7750_P4_REG32(SH7750_SDMR2_REGOFS)
> +#define SH7750_SDMR2_A7       SH7750_A7_REG32(SH7750_SDMR2_REGOFS)
> +
> +#define SH7750_SDMR3_REGOFS   0x940000       /* offset */
> +#define SH7750_SDMR3_REGNB    0x0FFC /* nb of register */
> +#define SH7750_SDMR3          SH7750_P4_REG32(SH7750_SDMR3_REGOFS)
> +#define SH7750_SDMR3_A7       SH7750_A7_REG32(SH7750_SDMR3_REGOFS)
> +
>  /*
>   * Direct Memory Access Controller (DMAC)
>   */





reply via email to

[Prev in Thread] Current Thread [Next in Thread]