[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64()
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 04/14] sdhci: use deposit64() |
Date: |
Thu, 14 Dec 2017 21:07:06 -0300 |
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.
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.
Regards,
Phil.
[Qemu-devel] [PATCH 03/14] sdhci: use the ldst_le_dma() API, Philippe Mathieu-Daudé, 2017/12/13
[Qemu-devel] [PATCH 05/14] sdhci: move MASK_TRNMOD with other SDHC_TRN* defines in "sd-internal.h", Philippe Mathieu-Daudé, 2017/12/13
[Qemu-devel] [PATCH 06/14] sdhci: refactor same sysbus/pci properties into a common one, Philippe Mathieu-Daudé, 2017/12/13