[v2] aspeed: misc fixes and enhancements (SMC)

[Qemu-devel] [PATCH v2 06/11] aspeed/smc: fix default read value

Posted by Cédric Le Goater 7 years, 1 month ago

0xFFFFFFFF should be returned for non implemented registers.

Also, the model should expose one control register per possible CS
even if there is no flash device attached. When testing the validity
of the register number in the read operation, replace 's->num_cs' by
'ctrl->max_slaves' which represents the maximum number of flash
devices a controller can handle.

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

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 1270842dcf0c..6045ca11b969 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -665,12 +665,12 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
-        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
+        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
         return s->regs[addr];
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
-        return 0;
+        return -1;
     }
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 06/11] aspeed/smc: fix default read value

Posted by Peter Maydell 7 years, 1 month ago

On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
> 0xFFFFFFFF should be returned for non implemented registers.
>
> Also,

Use of "Also" in a commit message often indicates that it
would be better to split the commit. The two changes here
don't seem to me to have much to do with each other.

> the model should expose one control register per possible CS
> even if there is no flash device attached. When testing the validity
> of the register number in the read operation, replace 's->num_cs' by
> 'ctrl->max_slaves' which represents the maximum number of flash
> devices a controller can handle.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 1270842dcf0c..6045ca11b969 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -665,12 +665,12 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>          addr == s->r_ce_ctrl ||
>          addr == R_INTR_CTRL ||
>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
> -        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
> +        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {

The commit message mentions changing the upper bound on the
address check here and also the unimplemented-register return
value, but this change also seems to be changing the lower bound
in the check ?

>          return s->regs[addr];
>      } else {
>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                        __func__, addr);
> -        return 0;
> +        return -1;
>      }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/11] aspeed/smc: fix default read value

Posted by Cédric Le Goater 7 years, 1 month ago

On 9/25/18 2:24 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater <clg@kaod.org> wrote:
>> 0xFFFFFFFF should be returned for non implemented registers.
>>
>> Also,
> 
> Use of "Also" in a commit message often indicates that it
> would be better to split the commit. The two changes here
> don't seem to me to have much to do with each other.

They do in the symptom which is to expose the correct register
values. But I won't argue and next version will introduce two 
patches :)

Thanks,

C.

>> the model should expose one control register per possible CS
>> even if there is no flash device attached. When testing the validity
>> of the register number in the read operation, replace 's->num_cs' by
>> 'ctrl->max_slaves' which represents the maximum number of flash
>> devices a controller can handle.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ssi/aspeed_smc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 1270842dcf0c..6045ca11b969 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -665,12 +665,12 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
>>          addr == s->r_ce_ctrl ||
>>          addr == R_INTR_CTRL ||
>>          (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
>> -        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
>> +        (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->ctrl->max_slaves)) {
> 
> The commit message mentions changing the upper bound on the
> address check here and also the unimplemented-register return
> value, but this change also seems to be changing the lower bound
> in the check ?
> 
>>          return s->regs[addr];
>>      } else {
>>          qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>>                        __func__, addr);
>> -        return 0;
>> +        return -1;
>>      }
> 
> thanks
> -- PMM
>