[Qemu-devel] [PATCH 0/3] aspeed/smc: small fixes

[Qemu-devel] [PATCH 0/3] aspeed/smc: small fixes

[Cédric Le Goater]

Hello,

Here is a short series of cleanups and fixes for issues in the Aspeed
SMC controller model discovered when experimenting with the MMIO exec
feature and also from tests under a QEMU PowerNV machine.

Indeed, the OPAL/skiboot firmware accesses the Aspeed SoC address
space using the iLPC->AHB bridge of the SuperIO controller and drives
the SPI controller to access the PNOR.

Thanks,

C.

Cédric Le Goater (3):
  aspeed/smc: fix dummy cycles count when in dual IO mode
  aspeed/smc: fix HW strapping
  aspeed/smc: rename aspeed_smc_flash_send_addr() to aspeed_smc_flash_setup()

 hw/ssi/aspeed_smc.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/3] aspeed/smc: fix dummy cycles count when in dual IO mode

[Cédric Le Goater]

When configured in dual I/O mode, address and data are sent in dual
mode, including the dummy byte cycles in between. Adapt the count to
the IO setting.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 5059396bc623..fce126e6ee92 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -66,6 +66,8 @@
 
 /* CEx Control Register */
 #define R_CTRL0           (0x10 / 4)
+#define   CTRL_IO_DUAL_DATA        (1 << 29)
+#define   CTRL_IO_DUAL_ADDR_DATA   (1 << 28) /* Includes dummies */
 #define   CTRL_CMD_SHIFT           16
 #define   CTRL_CMD_MASK            0xff
 #define   CTRL_DUMMY_HIGH_SHIFT    14
@@ -492,8 +494,13 @@ static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
     uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
     uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
     uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
+    uint32_t dummies = ((dummy_high << 2) | dummy_low) * 8;
 
-    return ((dummy_high << 2) | dummy_low) * 8;
+    if (r_ctrl0 & CTRL_IO_DUAL_ADDR_DATA) {
+        dummies /= 2;
+    }
+
+    return dummies;
 }
 
 static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
-- 
2.13.6

[Qemu-devel] [PATCH 2/3] aspeed/smc: fix HW strapping

[Cédric Le Goater]

Only the flash type is strapped by HW. The 4BYTE mode is set by
firmware when the flash device is detected.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index fce126e6ee92..b15370893583 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -639,23 +639,17 @@ static void aspeed_smc_reset(DeviceState *d)
             aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
     }
 
-    /* HW strapping for AST2500 FMC controllers  */
+    /* HW strapping flash type for FMC controllers  */
     if (s->ctrl->segments == aspeed_segments_ast2500_fmc) {
         /* flash type is fixed to SPI for CE0 and CE1 */
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
-
-        /* 4BYTE mode is autodetected for CE0. Let's force it to 1 for
-         * now */
-        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
     }
 
     /* HW strapping for AST2400 FMC controllers (SCU70). Let's use the
      * configuration of the palmetto-bmc machine */
     if (s->ctrl->segments == aspeed_segments_fmc) {
         s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
-
-        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
     }
 }
 
-- 
2.13.6

[Qemu-devel] [PATCH 3/3] aspeed/smc: rename aspeed_smc_flash_send_addr() to aspeed_smc_flash_setup()

[Cédric Le Goater]

Also handle the fake transfers for dummy bytes in this setup
routine. It will be useful when we activate MMIO execution.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index b15370893583..b29bfd3124a9 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -503,10 +503,11 @@ static int aspeed_smc_flash_dummies(const AspeedSMCFlash *fl)
     return dummies;
 }
 
-static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
+static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
 {
     const AspeedSMCState *s = fl->controller;
     uint8_t cmd = aspeed_smc_flash_cmd(fl);
+    int i;
 
     /* Flash access can not exceed CS segment */
     addr = aspeed_smc_check_segment_addr(fl, addr);
@@ -519,6 +520,18 @@ static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
     ssi_transfer(s->spi, (addr >> 16) & 0xff);
     ssi_transfer(s->spi, (addr >> 8) & 0xff);
     ssi_transfer(s->spi, (addr & 0xff));
+
+    /*
+     * Use fake transfers to model dummy bytes. The value should
+     * be configured to some non-zero value in fast read mode and
+     * zero in read mode. But, as the HW allows inconsistent
+     * settings, let's check for fast read mode.
+     */
+    if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
+        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
+                ssi_transfer(fl->controller->spi, 0xFF);
+        }
+    }
 }
 
 static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
@@ -537,19 +550,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
     case CTRL_READMODE:
     case CTRL_FREADMODE:
         aspeed_smc_flash_select(fl);
-        aspeed_smc_flash_send_addr(fl, addr);
-
-        /*
-         * Use fake transfers to model dummy bytes. The value should
-         * be configured to some non-zero value in fast read mode and
-         * zero in read mode. But, as the HW allows inconsistent
-         * settings, let's check for fast read mode.
-         */
-        if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
-            for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
-                ssi_transfer(fl->controller->spi, 0xFF);
-            }
-        }
+        aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
             ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
@@ -586,7 +587,7 @@ static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
         break;
     case CTRL_WRITEMODE:
         aspeed_smc_flash_select(fl);
-        aspeed_smc_flash_send_addr(fl, addr);
+        aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
             ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
-- 
2.13.6

Re: [Qemu-devel] [PATCH 0/3] aspeed/smc: small fixes

[Peter Maydell]

On 12 June 2018 at 07:57, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> Here is a short series of cleanups and fixes for issues in the Aspeed
> SMC controller model discovered when experimenting with the MMIO exec
> feature and also from tests under a QEMU PowerNV machine.
>
> Indeed, the OPAL/skiboot firmware accesses the Aspeed SoC address
> space using the iLPC->AHB bridge of the SuperIO controller and drives
> the SPI controller to access the PNOR.

Is there anybody familiar with the aspeed SoC who'd like to
review? The patches don't look particularly wrong, but I'm
not really in a position to be able to review...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] aspeed/smc: small fixes

[Cédric Le Goater]

On 06/20/2018 03:40 PM, Peter Maydell wrote:
> On 12 June 2018 at 07:57, Cédric Le Goater <clg@kaod.org> wrote:
>> Hello,
>>
>> Here is a short series of cleanups and fixes for issues in the Aspeed
>> SMC controller model discovered when experimenting with the MMIO exec
>> feature and also from tests under a QEMU PowerNV machine.
>>
>> Indeed, the OPAL/skiboot firmware accesses the Aspeed SoC address
>> space using the iLPC->AHB bridge of the SuperIO controller and drives
>> the SPI controller to access the PNOR.
> 
> Is there anybody familiar with the aspeed SoC who'd like to
> review? The patches don't look particularly wrong, but I'm
> not really in a position to be able to review...

Adding Andrew and Joel as I should have.

Thanks,

C.

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] aspeed/smc: fix dummy cycles count when in dual IO mode

[Andrew Jeffery]

On Tue, 12 Jun 2018, at 16:27, Cédric Le Goater wrote:
> When configured in dual I/O mode, address and data are sent in dual
> mode, including the dummy byte cycles in between. Adapt the count to
> the IO setting.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 5059396bc623..fce126e6ee92 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -66,6 +66,8 @@
>  
>  /* CEx Control Register */
>  #define R_CTRL0           (0x10 / 4)
> +#define   CTRL_IO_DUAL_DATA        (1 << 29)
> +#define   CTRL_IO_DUAL_ADDR_DATA   (1 << 28) /* Includes dummies */
>  #define   CTRL_CMD_SHIFT           16
>  #define   CTRL_CMD_MASK            0xff
>  #define   CTRL_DUMMY_HIGH_SHIFT    14
> @@ -492,8 +494,13 @@ static int aspeed_smc_flash_dummies(const 
> AspeedSMCFlash *fl)
>      uint32_t r_ctrl0 = s->regs[s->r_ctrl0 + fl->id];
>      uint32_t dummy_high = (r_ctrl0 >> CTRL_DUMMY_HIGH_SHIFT) & 0x1;
>      uint32_t dummy_low = (r_ctrl0 >> CTRL_DUMMY_LOW_SHIFT) & 0x3;
> +    uint32_t dummies = ((dummy_high << 2) | dummy_low) * 8;
>  
> -    return ((dummy_high << 2) | dummy_low) * 8;
> +    if (r_ctrl0 & CTRL_IO_DUAL_ADDR_DATA) {
> +        dummies /= 2;
> +    }
> +
> +    return dummies;
>  }
>  
>  static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
> -- 
> 2.13.6
> 
> 

Re: [Qemu-devel] [PATCH 2/3] aspeed/smc: fix HW strapping

[Andrew Jeffery]

On Tue, 12 Jun 2018, at 16:27, Cédric Le Goater wrote:
> Only the flash type is strapped by HW. The 4BYTE mode is set by
> firmware when the flash device is detected.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index fce126e6ee92..b15370893583 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -639,23 +639,17 @@ static void aspeed_smc_reset(DeviceState *d)
>              aspeed_smc_segment_to_reg(&s->ctrl->segments[i]);
>      }
>  
> -    /* HW strapping for AST2500 FMC controllers  */
> +    /* HW strapping flash type for FMC controllers  */
>      if (s->ctrl->segments == aspeed_segments_ast2500_fmc) {
>          /* flash type is fixed to SPI for CE0 and CE1 */
>          s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
>          s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE1);
> -
> -        /* 4BYTE mode is autodetected for CE0. Let's force it to 1 for
> -         * now */
> -        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
>      }
>  
>      /* HW strapping for AST2400 FMC controllers (SCU70). Let's use the
>       * configuration of the palmetto-bmc machine */
>      if (s->ctrl->segments == aspeed_segments_fmc) {
>          s->regs[s->r_conf] |= (CONF_FLASH_TYPE_SPI << CONF_FLASH_TYPE0);
> -
> -        s->regs[s->r_ce_ctrl] |= (1 << (CTRL_EXTENDED0));
>      }
>  }
>  
> -- 
> 2.13.6
> 
> 

Re: [Qemu-devel] [PATCH 3/3] aspeed/smc: rename aspeed_smc_flash_send_addr() to aspeed_smc_flash_setup()

[Andrew Jeffery]

On Tue, 12 Jun 2018, at 16:27, Cédric Le Goater wrote:
> Also handle the fake transfers for dummy bytes in this setup
> routine. It will be useful when we activate MMIO execution.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  hw/ssi/aspeed_smc.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index b15370893583..b29bfd3124a9 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -503,10 +503,11 @@ static int aspeed_smc_flash_dummies(const 
> AspeedSMCFlash *fl)
>      return dummies;
>  }
>  
> -static void aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
> +static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>  {
>      const AspeedSMCState *s = fl->controller;
>      uint8_t cmd = aspeed_smc_flash_cmd(fl);
> +    int i;
>  
>      /* Flash access can not exceed CS segment */
>      addr = aspeed_smc_check_segment_addr(fl, addr);
> @@ -519,6 +520,18 @@ static void 
> aspeed_smc_flash_send_addr(AspeedSMCFlash *fl, uint32_t addr)
>      ssi_transfer(s->spi, (addr >> 16) & 0xff);
>      ssi_transfer(s->spi, (addr >> 8) & 0xff);
>      ssi_transfer(s->spi, (addr & 0xff));
> +
> +    /*
> +     * Use fake transfers to model dummy bytes. The value should
> +     * be configured to some non-zero value in fast read mode and
> +     * zero in read mode. But, as the HW allows inconsistent
> +     * settings, let's check for fast read mode.
> +     */
> +    if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
> +        for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> +                ssi_transfer(fl->controller->spi, 0xFF);
> +        }
> +    }
>  }
>  
>  static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, 
> unsigned size)
> @@ -537,19 +550,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, 
> hwaddr addr, unsigned size)
>      case CTRL_READMODE:
>      case CTRL_FREADMODE:
>          aspeed_smc_flash_select(fl);
> -        aspeed_smc_flash_send_addr(fl, addr);
> -
> -        /*
> -         * Use fake transfers to model dummy bytes. The value should
> -         * be configured to some non-zero value in fast read mode and
> -         * zero in read mode. But, as the HW allows inconsistent
> -         * settings, let's check for fast read mode.
> -         */
> -        if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
> -            for (i = 0; i < aspeed_smc_flash_dummies(fl); i++) {
> -                ssi_transfer(fl->controller->spi, 0xFF);
> -            }
> -        }
> +        aspeed_smc_flash_setup(fl, addr);
>  
>          for (i = 0; i < size; i++) {
>              ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
> @@ -586,7 +587,7 @@ static void aspeed_smc_flash_write(void *opaque, 
> hwaddr addr, uint64_t data,
>          break;
>      case CTRL_WRITEMODE:
>          aspeed_smc_flash_select(fl);
> -        aspeed_smc_flash_send_addr(fl, addr);
> +        aspeed_smc_flash_setup(fl, addr);
>  
>          for (i = 0; i < size; i++) {
>              ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
> -- 
> 2.13.6
> 
> 

Re: [Qemu-devel] [PATCH 0/3] aspeed/smc: small fixes

[Peter Maydell]

On 12 June 2018 at 07:57, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> Here is a short series of cleanups and fixes for issues in the Aspeed
> SMC controller model discovered when experimenting with the MMIO exec
> feature and also from tests under a QEMU PowerNV machine.
>
> Indeed, the OPAL/skiboot firmware accesses the Aspeed SoC address
> space using the iLPC->AHB bridge of the SuperIO controller and drives
> the SPI controller to access the PNOR.
>
> Thanks,
>
> C.
>
> Cédric Le Goater (3):
>   aspeed/smc: fix dummy cycles count when in dual IO mode
>   aspeed/smc: fix HW strapping
>   aspeed/smc: rename aspeed_smc_flash_send_addr() to aspeed_smc_flash_setup()



Applied to target-arm.next, thanks.

-- PMM