On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > This makes the code slightly safer, also easier to review. > > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> > --- > hw/sd/sdhci.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index e39623baba..295a89e5d3 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t > val, unsigned size) > MASKED_WRITE(s->admaerr, mask, value); > break; > case SDHC_ADMASYSADDR: > - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL | > - (uint64_t)mask)) | (uint64_t)value; > + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
This doesn't look right. Originally we were masking admasysaddr with (mask and 0xFFFFFFFF00000000). Then ORing in the value. Now we are depositing value into a bit field that starts at bit 32 and has 0 length. I don't see how value will be deposited at all with a 0 length. Alistair > break; > case SDHC_ADMASYSADDR + 4: > - s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL | > - ((uint64_t)mask << 32))) | ((uint64_t)value << 32); > + s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value); > break; > case SDHC_FEAER: > s->acmd12errsts |= value; > -- > 2.15.1 > >
