qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()


From: Alistair Francis
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()
Date: Thu, 14 Dec 2017 17:14:40 -0800

On Thu, Dec 14, 2017 at 4:07 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
> On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis
> <address@hidden> wrote:
>> On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <address@hidden> 
>> 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.
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]