From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, HTML_MESSAGE,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D539C43603 for ; Mon, 16 Dec 2019 21:29:23 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id ED3B02465E for ; Mon, 16 Dec 2019 21:29:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org ED3B02465E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=amsat.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:60192 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1igxve-0005SC-0W for qemu-devel@archiver.kernel.org; Mon, 16 Dec 2019 16:29:22 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:50022) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1igxuo-0004v9-OH for qemu-devel@nongnu.org; Mon, 16 Dec 2019 16:28:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1igxun-0004Kl-4S for qemu-devel@nongnu.org; Mon, 16 Dec 2019 16:28:30 -0500 Received: from mail-yw1-f68.google.com ([209.85.161.68]:38366) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1igxuj-0004Id-Fe; Mon, 16 Dec 2019 16:28:25 -0500 Received: by mail-yw1-f68.google.com with SMTP id 10so3101801ywv.5; Mon, 16 Dec 2019 13:28:25 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hngYlw1ZLnncQM83R3mqcAXHiBFV3HcVdNJ/QZK7/Kc=; b=Xc9JuvpzJnt+eWOeYz98a1xNDhVh4EHaNpfc3NlC9F6wX6zdJ12QxS7FPkyKEwol2U uAmAdZxETSkAtxFb0QOLVsr48905CL8ucwQowxIIocE4bcNL5HP2SkIiDBwlRHPxDnz/ lViV9oyeYbQzvyxch/pidgQsv3kYECJ7+lI2Wyuj9ZWdqVzew1QUOnTtdUdxySDq3jAa ywcWz4H60w7QICSCZotozKCHa/QM7qNc88VNqsq1ERHxlN5+eNby/+fJZLFaEGsrug8l SzwpxgRhgfVbdM+Sj1QUnVq4ZcA9mVbPSpI+W8PeLhj9cOu6kL6HVMIQ/WeigX3DvmlZ +IKQ== X-Gm-Message-State: APjAAAXy/S/4AW7HrL7ReczZL3z30+Tcik9ImUJbwatEKGrqIapvBCDZ ff8tP2SyRCltkmCPupt0YNPksVo7L7qNvLosuiM= X-Google-Smtp-Source: APXvYqwg+gh4zL4+SjgxHYG2P16RV3uXk/AMrtS8Ilb9Tn9I0MPvFGEqMvYput+rYPXAZXf4/cYMPLr7c1gnmiLG2bE= X-Received: by 2002:a81:3ad0:: with SMTP id h199mr8106449ywa.37.1576531704611; Mon, 16 Dec 2019 13:28:24 -0800 (PST) MIME-Version: 1.0 References: <20191202210947.3603-1-nieklinnenbank@gmail.com> <20191202210947.3603-10-nieklinnenbank@gmail.com> <6bee15d7-7d80-0709-ac90-ef2052b39329@redhat.com> <03a78f1d-e8fe-5a53-b061-d39de9ed7a9e@redhat.com> In-Reply-To: From: =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Date: Mon, 16 Dec 2019 22:28:12 +0100 Message-ID: Subject: Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller To: Niek Linnenbank Content-Type: multipart/alternative; boundary="0000000000003b09be0599d8e485" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.161.68 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Beniamino Galvani , Peter Maydell , qemu-arm , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , QEMU Developers Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000003b09be0599d8e485 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Le lun. 16 d=C3=A9c. 2019 20:46, Niek Linnenbank = a =C3=A9crit : > > > On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daud=C3=A9 > wrote: > >> On 12/16/19 12:07 AM, Niek Linnenbank wrote: >> > >> > >> > On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daud=C3=A9 >> > > wrote: >> > >> > Hi Niek, >> > >> > On 12/11/19 11:34 PM, Niek Linnenbank wrote: >> [...] >> > > +static uint32_t aw_h3_sdhost_process_desc(AwH3SDHostState >> *s, >> > > + hwaddr desc_addr= , >> > > + TransferDescript= or >> > *desc, >> > > + bool is_write, >> > uint32_t >> > > max_bytes) >> > > +{ >> > > + uint32_t num_done =3D 0; >> > > + uint32_t num_bytes =3D max_bytes; >> > > + uint8_t buf[1024]; >> > > + >> > > + /* Read descriptor */ >> > > + cpu_physical_memory_read(desc_addr, desc, >> sizeof(*desc)); >> > >> > Should we worry about endianess here? >> > >> > >> > I tried to figure out what is expected, but the >> > Allwinner_H3_Datasheet_V1.2.pdf does not >> > explicitly mention endianness for any of its I/O devices. Currently it >> > seems all devices are >> > happy with using the same endianness as the CPUs. In the >> MemoryRegionOps >> > has DEVICE_NATIVE_ENDIAN >> > set to match the behavior seen. >> >> OK. >> >> [...] >> > > +static const MemoryRegionOps aw_h3_sdhost_ops =3D { >> > > + .read =3D aw_h3_sdhost_read, >> > > + .write =3D aw_h3_sdhost_write, >> > > + .endianness =3D DEVICE_NATIVE_ENDIAN, >> > >> > I haven't checked .valid accesses from the datasheet. >> > >> > However due to: >> > >> > res =3D s->data_crc[((offset - REG_SD_DATA7_CRC) / >> sizeof(uint32_t))]; >> > >> > You seem to expect: >> > >> > .impl.min_access_size =3D 4, >> > >> > .impl.max_access_size unset is 8, which should works. >> > >> > It seems that all registers are aligned on at least 32-bit boundaries. >> > And the section 5.3.5.1 mentions >> > that the DMA descriptors must be stored in memory 32-bit aligned. So >> > based on that information, >> >> So you are describing ".valid.min_access_size =3D 4", which is the minim= um >> access size on the bus. >> ".impl.min_access_size" is different, it is what access sizes is ready >> to handle your model. >> Your model read/write handlers expect addresses aligned on 32-bit >> boundary, this is why I suggested to use ".impl.min_access_size =3D 4". = If >> the guest were using a 16-bit access, your model would be buggy. If you >> describe your implementation to accept minimum 32-bit and the guest is >> allowed to use smaller accesses, QEMU will do a 32-bit access to the >> device, and return the 16-bit part to the guest. This way your model is >> safe. This is done by access_with_adjusted_size() in memory.c. >> If you restrict with ".valid.min_access_size =3D 4", you might think we >> don't need ".valid.min_access_size =3D 4" because all access from guest >> will be at least 32-bit. However keep in mind someone might find this >> device in another datasheet not limited to 32-bit, and let's say change >> to ".valid.min_access_size =3D 2". Without ".impl.min_access_size =3D 4" >> your model is buggy. So to be safe I'd use: >> >> .impl.min_access_size =3D 4, >> .valid.min_access_size =3D 4, >> > > Now it makes more sense to me, thanks Philippe for explaining this! > Great, I'll add .impl.min_access_size =3D 4. > > At this point, I've processed all the feedback that I received for all of > the patches > in this series. Is there anything else you would like to > see/discuss/review, or shall I send the v2 when I finish testing? > Send it! We'll discuss on updated v2 :) Regards, Phil. --0000000000003b09be0599d8e485 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Le lun. 16 d=C3=A9c. 2019 20:46, Niek Linnenbank <nieklinnenbank@gmail.com> a =C3= =A9crit=C2=A0:


On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daud=C3= =A9 <philmd@redhat.com> wrote:
On 12/16/19 12:07 AM, Niek Linnenbank wrote:
>
>
> On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daud=C3=A9
> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote= :
>
>=C2=A0 =C2=A0 =C2=A0Hi Niek,
>
>=C2=A0 =C2=A0 =C2=A0On 12/11/19 11:34 PM, Niek Linnenbank wrote:
[...]
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+static uint32_t aw_h3_sdh= ost_process_desc(AwH3SDHostState *s,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hwaddr desc_addr,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 TransferDescriptor
>=C2=A0 =C2=A0 =C2=A0*desc,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 bool is_write,
>=C2=A0 =C2=A0 =C2=A0uint32_t
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0max_bytes)
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+{
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 uint32_t nu= m_done =3D 0;
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 uint32_t nu= m_bytes =3D max_bytes;
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 uint8_t buf= [1024];
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 /* Read des= criptor */
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 cpu_physica= l_memory_read(desc_addr, desc, sizeof(*desc));
>
>=C2=A0 =C2=A0 =C2=A0Should we worry about endianess here?
>
>
> I tried to figure out what is expected, but the
> Allwinner_H3_Datasheet_V1.2.pdf does not
> explicitly mention endianness for any of its I/O devices. Currently it=
> seems all devices are
> happy with using the same endianness as the CPUs. In the MemoryRegionO= ps
> has DEVICE_NATIVE_ENDIAN
> set to match the behavior seen.

OK.

[...]
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+static const MemoryRegion= Ops aw_h3_sdhost_ops =3D {
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 .read =3D a= w_h3_sdhost_read,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 .write =3D = aw_h3_sdhost_write,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0+=C2=A0 =C2=A0 .endianness= =3D DEVICE_NATIVE_ENDIAN,
>
>=C2=A0 =C2=A0 =C2=A0I haven't checked .valid accesses from the data= sheet.
>
>=C2=A0 =C2=A0 =C2=A0However due to:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0res =3D s->data_crc[((offset - REG= _SD_DATA7_CRC) / sizeof(uint32_t))];
>
>=C2=A0 =C2=A0 =C2=A0You seem to expect:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .impl.mi= n_access_size =3D 4,
>
>=C2=A0 =C2=A0 =C2=A0.impl.max_access_size unset is 8, which should work= s.
>
> It seems that all registers are aligned on at least 32-bit boundaries.=
> And the section 5.3.5.1 mentions
> that the DMA descriptors must be stored in memory 32-bit aligned. So <= br> > based on that information,

So you are describing ".valid.min_access_size =3D 4", which is th= e minimum
access size on the bus.
".impl.min_access_size" is different, it is what access sizes is = ready
to handle your model.
Your model read/write handlers expect addresses aligned on 32-bit
boundary, this is why I suggested to use ".impl.min_access_size =3D 4&= quot;. If
the guest were using a 16-bit access, your model would be buggy. If you describe your implementation to accept minimum 32-bit and the guest is
allowed to use smaller accesses, QEMU will do a 32-bit access to the
device, and return the 16-bit part to the guest. This way your model is safe. This is done by access_with_adjusted_size() in memory.c.
If you restrict with ".valid.min_access_size =3D 4", you might th= ink we
don't need ".valid.min_access_size =3D 4" because all access = from guest
will be at least 32-bit. However keep in mind someone might find this
device in another datasheet not limited to 32-bit, and let's say change=
to ".valid.min_access_size =3D 2". Without ".impl.min_access= _size =3D 4"
your model is buggy. So to be safe I'd use:

=C2=A0 =C2=A0.impl.min_access_size =3D 4,
=C2=A0 =C2=A0.valid.min_access_size =3D 4,

<= div>Now it makes more sense to me, thanks Philippe for explaining this!
=
Great, I'll add .impl.min_access_size =3D 4.

<= /div>
At this point, I've processed all the feedback that I receive= d for all of the patches
in this series. Is there anything else y= ou would like to see/discuss/review, or shall I send the v2 when I finish t= esting?
Send it! We'll discuss on updated v2 :)
<= div dir=3D"auto">
Regards,=C2=A0

Phil.
--0000000000003b09be0599d8e485--