| Message ID | 20180921161939.822-7-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed: misc fixes and enhancements (SMC) | expand |
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
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 >
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; } }
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(-)