On Thu, Dec 14, 2017 at 4:07 PM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis > <alistair.fran...@xilinx.com> wrote: >> 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. > > 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. Alistair > > Regards, > > Phil. >