On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: >>> @@ -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. > > good catch :) I'll respin with: > > case SDHC_ADMASYSADDR: > s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value) > break; > case SDHC_ADMASYSADDR + 4: > s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value); > break;
This still doesn't take the mask value into account though. Also, doesn't deposit() shift value up in this case? We want to mask the low bits out. I don't have the code in front of me though, so I could be wrong here. 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; >