Ping!
Igor, are you able to provide a diff of this patch so I can send the
next revision?
Regards,
Peter
On Mon, 2012-08-06 at 16:29 +0400, Igor Mitsyanko wrote:
> On 08/06/2012 02:56 PM, Peter Maydell wrote:
> > On 6 August 2012 04:25, Peter A. G. Crosthwaite
> > <peter.crosthwa...@petalogix.com> wrote:
> >
> >> +static uint64_t
> >> +exynos4210_sdhci_readfn(void *opaque, target_phys_addr_t offset, unsigned
> >> size)
> >> +{
> >> + Exynos4SDHCIState *s = (Exynos4SDHCIState *)opaque;
> >> + uint32_t ret;
> >> +
> >> + switch (offset & ~0x3) {
> >> + case SDHC_BDATA:
> >> + /* Buffer data port read can be disabled by CONTROL2 register */
> >> + if (s->control2 & EXYNOS4_SDHC_DISBUFRD) {
> >> + ret = 0;
> >> + } else {
> >> + ret = SDHCI_GET_CLASS(s)->mem_read(SDHCI(s), offset, size);
> >> + }
> >> + break;
> >> + case SDHC_ADMAERR:
> >> + ret = (s->admaerr >> 8 * (offset - SDHC_ADMAERR)) &
> >> + ((1 << 8 * size) - 1);
> > If size == 4 you've just shifted right by 32, which is undefined behaviour
> > when ints are 32 bits. Try
> >
> > ret = extract32(s->admaerr, (offset & 3) << 3, size * 8);
> >
> > and similarly below.
>
> Ok
>
> >> +static void exynos4210_sdhci_writefn(void *opaque, target_phys_addr_t
> >> offset,
> >> + uint64_t val, unsigned size)
> >> +{
> >> + Exynos4SDHCIState *s = (Exynos4SDHCIState *)opaque;
> >> + SDHCIState *sdhci = SDHCI(s);
> >> + unsigned shift;
> >> +
> >> + DPRINT_L2("write %ub: addr[0x%04x] <- %u(0x%x)\n", size,
> >> (uint32_t)offset,
> >> + (uint32_t)val, (uint32_t)val);
> >> +
> >> + switch (offset) {
> >> + case SDHC_CLKCON:
> >> + if ((val & SDHC_CLOCK_SDCLK_EN) &&
> >> + (sdhci->prnsts & SDHC_CARD_PRESENT)) {
> >> + val |= EXYNOS4_SDHC_SDCLK_STBL;
> >> + } else {
> >> + val &= ~EXYNOS4_SDHC_SDCLK_STBL;
> >> + }
> >> + /* Break out to superclass write to handle the rest of this
> >> register */
> >> + break;
> >> + case EXYNOS4_SDHC_CONTROL2 ... EXYNOS4_SDHC_CONTROL2 + 3:
> > Why do we switch (offset & 3) in the readfn but switch (offset)
> > and use case FOO ... FOO + 3 in the writefn? Consistency would be
> > nice.
>
> I think I'll change readfn() switch to match writefn then, to avoid
> complicating SDHC_CLKON case.
>
> >> + shift = (offset - EXYNOS4_SDHC_CONTROL2) * 8;
> >> + s->control2 = (s->control2 & ~(((1 << 8 * size) - 1) << shift)) |
> >> + (val << shift);
> > s->control2 = deposit32(s->control2, (offset & 3) << 3, size * 8, val);
> >
> > and similarly below.
> >
> >> + case SDHC_ADMAERR ... SDHC_ADMAERR + 3:
> >> + if (size == 4 || (size == 2 && offset == SDHC_ADMAERR) ||
> >> + (size == 1 && offset == (SDHC_ADMAERR + 1))) {
> >> + uint32_t mask = 0;
> >> +
> >> + if (size == 2) {
> >> + mask = 0xFFFF0000;
> >> + } else if (size == 1) {
> >> + mask = 0xFFFF00FF;
> >> + val <<= 8;
> >> + }
> >> +
> >> + s->admaerr = (s->admaerr & (mask | EXYNOS4_SDHC_FINAL_BLOCK |
> >> + EXYNOS4_SDHC_IRQ_STAT)) | (val &
> >> ~(EXYNOS4_SDHC_FINAL_BLOCK |
> >> + EXYNOS4_SDHC_IRQ_STAT | EXYNOS4_SDHC_CONTINUE_REQ));
> >> + s->admaerr &= ~(val & EXYNOS4_SDHC_IRQ_STAT);
> >> + if ((s->stopped_adma) && (val & EXYNOS4_SDHC_CONTINUE_REQ) &&
> >> + (SDHC_DMA_TYPE(sdhci->hostctl) == SDHC_CTRL_ADMA2_32)) {
> >> + s->stopped_adma = false;
> >> + SDHCI_GET_CLASS(sdhci)->do_adma(sdhci);
> >> + }
> >> + } else {
> >> + uint32_t mask = (1 << (size * 8)) - 1;
> >> + shift = 8 * (offset & 0x3);
> >> + val <<= shift;
> >> + mask = ~(mask << shift);
> >> + s->admaerr = (s->admaerr & mask) | val;
> >> + }
> >> + return;
> > This case just looks odd. I think it would be clearer to first
> > calculate the updated value of admaerr (using deposit32) and
> > then act on the changes (xor of old and new value is handy
> > to identify which bits are changed).
>
> ok
> >
> > -- PMM
> >
>
