[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH 04/14] sdhci: use deposit64() |
Date: |
Thu, 14 Dec 2017 22:51:29 -0300 |
>>>> 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);
}