QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: "Beniamino Galvani" <b.galvani@gmail.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller
Date: Mon, 16 Dec 2019 22:28:12 +0100
Message-ID: <CAAdtpL6Vik9zVwSpVshWWicNMNitJ--0Xz7WHxd_wOLNGNkwgg@mail.gmail.com> (raw)
In-Reply-To: <CAPan3WrHQk-apQhQrihF_71b3_PSqkaEu1dHYmeCXuygwnAy2A@mail.gmail.com>


[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

Le lun. 16 déc. 2019 20:46, Niek Linnenbank <nieklinnenbank@gmail.com> a
écrit :

>
>
> On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé <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é
>> > <philmd@redhat.com <mailto:philmd@redhat.com>> 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,
>> >      >     +                                          TransferDescriptor
>> >     *desc,
>> >      >     +                                          bool is_write,
>> >     uint32_t
>> >      >     max_bytes)
>> >      >     +{
>> >      >     +    uint32_t num_done = 0;
>> >      >     +    uint32_t num_bytes = 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 = {
>> >      >     +    .read = aw_h3_sdhost_read,
>> >      >     +    .write = aw_h3_sdhost_write,
>> >      >     +    .endianness = DEVICE_NATIVE_ENDIAN,
>> >
>> >     I haven't checked .valid accesses from the datasheet.
>> >
>> >     However due to:
>> >
>> >         res = s->data_crc[((offset - REG_SD_DATA7_CRC) /
>> sizeof(uint32_t))];
>> >
>> >     You seem to expect:
>> >
>> >                  .impl.min_access_size = 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 = 4", which is the 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 = 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 = 4", you might think we
>> don't need ".valid.min_access_size = 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 = 2". Without ".impl.min_access_size = 4"
>> your model is buggy. So to be safe I'd use:
>>
>>    .impl.min_access_size = 4,
>>    .valid.min_access_size = 4,
>>
>
> Now it makes more sense to me, thanks Philippe for explaining this!
> Great, I'll add .impl.min_access_size = 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.

[-- Attachment #2: Type: text/html, Size: 5608 bytes --]

  reply index

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 21:09 [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Niek Linnenbank
2019-12-02 21:09 ` [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip Niek Linnenbank
2019-12-04 16:53   ` Philippe Mathieu-Daudé
2019-12-04 20:44     ` Niek Linnenbank
2019-12-10  9:02   ` Philippe Mathieu-Daudé
2019-12-10 19:17     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine Niek Linnenbank
2019-12-03  9:17   ` Philippe Mathieu-Daudé
2019-12-03 19:33     ` Niek Linnenbank
2019-12-04  9:03       ` Philippe Mathieu-Daudé
2019-12-04 19:50         ` Niek Linnenbank
2019-12-05 22:15     ` Niek Linnenbank
2019-12-06  5:41       ` Philippe Mathieu-Daudé
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé
2019-12-10 19:14             ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 03/10] arm: allwinner-h3: add Clock Control Unit Niek Linnenbank
2019-12-13  0:03   ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 04/10] arm: allwinner-h3: add USB host controller Niek Linnenbank
2019-12-04 16:11   ` Aleksandar Markovic
2019-12-04 20:20     ` Niek Linnenbank
2019-12-10  7:56   ` Philippe Mathieu-Daudé
2019-12-10  8:29     ` Gerd Hoffmann
2019-12-10 19:11       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 05/10] arm: allwinner-h3: add System Control module Niek Linnenbank
2019-12-13  0:09   ` Philippe Mathieu-Daudé
2019-12-15 23:27     ` Niek Linnenbank
2019-12-16  0:17       ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on() Niek Linnenbank
2019-12-06 14:24   ` Peter Maydell
2019-12-06 20:01     ` Niek Linnenbank
2019-12-13 20:52       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 07/10] arm: allwinner-h3: add CPU Configuration module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 08/10] arm: allwinner-h3: add Security Identifier device Niek Linnenbank
2019-12-06 14:27   ` Peter Maydell
2019-12-06 16:35     ` Philippe Mathieu-Daudé
2019-12-06 20:20       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller Niek Linnenbank
2019-12-11 22:34   ` Niek Linnenbank
2019-12-12 23:56     ` Philippe Mathieu-Daudé
2019-12-13 21:00       ` Niek Linnenbank
2019-12-14 13:59         ` Philippe Mathieu-Daudé
2019-12-14 20:32           ` Niek Linnenbank
2019-12-15 23:07       ` Niek Linnenbank
2019-12-16  0:14         ` Philippe Mathieu-Daudé
2019-12-16 19:46           ` Niek Linnenbank
2019-12-16 21:28             ` Philippe Mathieu-Daudé [this message]
2019-12-02 21:09 ` [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device Niek Linnenbank
2019-12-03  9:33   ` KONRAD Frederic
2019-12-03 19:41     ` Niek Linnenbank
2019-12-04 15:14     ` Philippe Mathieu-Daudé
2019-12-04 15:22       ` KONRAD Frederic
2019-12-03  8:47 ` [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Philippe Mathieu-Daudé
2019-12-03 19:25   ` Niek Linnenbank
2019-12-10  8:40     ` Philippe Mathieu-Daudé
2019-12-09 21:37   ` Niek Linnenbank
2019-12-10  8:26     ` Philippe Mathieu-Daudé
2019-12-10 20:12       ` Niek Linnenbank
2019-12-12 23:07         ` Niek Linnenbank
2019-12-12 23:25           ` Philippe Mathieu-Daudé
2019-12-13 20:45             ` Niek Linnenbank
2019-12-03  9:02 ` Philippe Mathieu-Daudé
2019-12-03 19:32   ` Niek Linnenbank
2019-12-06 14:16     ` Peter Maydell
2019-12-09 22:24       ` Aleksandar Markovic
2019-12-10 10:34 ` KONRAD Frederic
2019-12-10 19:55   ` Niek Linnenbank

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAAdtpL6Vik9zVwSpVshWWicNMNitJ--0Xz7WHxd_wOLNGNkwgg@mail.gmail.com \
    --to=f4bug@amsat.org \
    --cc=b.galvani@gmail.com \
    --cc=nieklinnenbank@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git