>>>> 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. >> >> We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits. > > Ah! Good point. > >> Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit. >> >> /** >> * Deposit @fieldval into the 64 bit @value at the bit field specified >> * by the @start and @length parameters, and return the modified >> * @value. Bits of @value outside the bit field are not modified. >> >> uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval); >> >> in both access we use length=32 >> >> at SDHC_ADMASYSADDR we use start=0, >> while at SDHC_ADMASYSADDR + 4 we use start=32. >> >> both deposit the 32b value (32b masked) into a 64b s->admasysaddr. >> >> This is good to clarify this now, because the Spec v3 series (and >> v4.20 if we want it) add a lot of them. > > Ok, this sounds fine to me then. > > The mask variable is still being ignored though. value should be anded > with mask.
This is what deposit64() does: uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval) { uint64_t mask; assert(start >= 0 && length > 0 && length <= 64 - start); mask = (~0ULL >> (64 - length)) << start; return (value & ~mask) | ((fieldval << start) & mask); }