From: Bernhard Beschow <shentey@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
"Aurelien Jarno" <aurelien@aurel32.net>,
"Yoshinori Sato" <ysato@users.sourceforge.jp>,
"Antony Pavlov" <antonynpavlov@gmail.com>,
"BALATON Zoltan" <balaton@eik.bme.hu>,
"Alistair Francis" <alistair@alistair23.me>,
"Bin Meng" <bin.meng@windriver.com>,
"Kevin Wolf" <kwolf@redhat.com>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Jan Kiszka" <jan.kiszka@web.de>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
"Hanna Reitz" <hreitz@redhat.com>,
qemu-arm@nongnu.org, "Magnus Damm" <magnus.damm@gmail.com>,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
qemu-block@nongnu.org
Subject: Re: [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model
Date: Fri, 16 Dec 2022 14:38:28 +0000 [thread overview]
Message-ID: <BEA593C6-8569-44AE-8E4B-554B6D389B72@gmail.com> (raw)
In-Reply-To: <6F4D0C8A-5DDC-4802-BE98-F546E6E73048@gmail.com>
Am 1. November 2022 10:49:20 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Am 31. Oktober 2022 12:11:37 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>On 30/10/22 12:46, Bernhard Beschow wrote:
>>>
>>>
>>> On Sun, Oct 30, 2022 at 1:10 AM Philippe Mathieu-Daudé <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:
>>>
>>> On 29/10/22 20:28, Bernhard Beschow wrote:
>>> > Am 29. Oktober 2022 13:04:00 UTC schrieb Bernhard Beschow
>>> <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>> >> Am 29. Oktober 2022 11:33:51 UTC schrieb Bernhard Beschow
>>> <shentey@gmail.com <mailto:shentey@gmail.com>>:
>>> >>> Am 27. Oktober 2022 21:40:01 UTC schrieb "Philippe
>>> Mathieu-Daudé" <philmd@linaro.org <mailto:philmd@linaro.org>>:
>>> >>>> Hi Bernhard,
>>> >>>>
>>> >>>> On 18/10/22 23:01, Bernhard Beschow wrote:
>>> >>>>> Will allow e500 boards to access SD cards using just their
>>> own devices.
>>> >>>>>
>>> >>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com
>>> <mailto:shentey@gmail.com>>
>>> >>>>> ---
>>> >>>>> hw/sd/sdhci.c | 120
>>> +++++++++++++++++++++++++++++++++++++++++-
>>> >>>>> include/hw/sd/sdhci.h | 3 ++
>>> >>>>> 2 files changed, 122 insertions(+), 1 deletion(-)
>>>
>>> >>>> So now, I'd create 1 UNIMP region for ESDHC_WML and map it
>>> >>>> into SDHC_REGISTERS_MAP (s->iomem) with priority 1, and add
>>> >>>> another UNIMP region of ESDHC_REGISTERS_MAP_SIZE -
>>> SDHC_REGISTERS_MAP_SIZE (= 0x310) and map it normally at offset
>>> >>>> 0x100 (SDHC_REGISTERS_MAP_SIZE). Look at create_unimp() in
>>> >>>> hw/arm/bcm2835_peripherals.c.
>>> >>>>
>>> >>>> But the ESDHC_WML register has address 0x44 and fits inside the
>>> >>>> SDHC_REGISTERS_MAP region, so likely belong there. 0x44 is the
>>> >>>> upper part of the SDHC_CAPAB register. These bits are undefined
>>> >>>> on the spec v2, which I see you are setting in esdhci_init().
>>> >>>> So this register should already return 0, otherwise we have
>>> >>>> a bug. Thus we don't need to handle this ESDHC_WML particularly.
>>> >>
>>> >> My idea here was to catch this unimplemented case in order to
>>> indicate this clearly to users. Perhaps it nudges somebody to
>>> provide a patch?
>>> >>
>>> >>>>
>>> >>>> And your model is reduced to handling create_unimp() in
>>> esdhci_realize().
>>> >>>>
>>> >>>> Am I missing something?
>>> >>>
>>> >>> The mmio ops are big endian and need to be aligned to a 4-byte
>>> boundary. It took me quite a while to debug this. So shall I just
>>> create an additional memory region for the region above
>>> SDHC_REGISTERS_MAP_SIZE for ESDHC_DMA_SYSCTL?
>>> >>
>>> >> All in all I currently don't have a better idea than keeping the
>>> custom i/o ops for the standard region and adding an additional
>>> unimplemented region for ESDHC_DMA_SYSCTL. I think I'd have to
>>> dynamically allocate memory for it where I still need to figure out
>>> how not to leak it.
>>> >
>>> > By simply reusing sdhci_{read,write} in eSDHC's io_ops struct I
>>> was able to remove the custom implementations while having big
>>> endian and the alignments proper. However, I don't see a way of
>>> adding two memory regions - with or without a container. With a
>>> container I'd have to somehow preserve the mmio attribute which is
>>> initialized by the parent class, re-initialize it with the
>>> container, and add the preserved memory region as child. This seems
>>> very fragile, esp. since the parent class has created an alias for
>>> mmio in sysbus. Without a container, one would have two memory
>>> regions that both have to be mapped separately by the caller, i.e.
>>> it burdens the caller with an implementation detail.
>>> >
>>> > Any suggestions?
>>
>>See https://lore.kernel.org/qemu-devel/20221031115402.91912-7-philmd@linaro.org/
>>
>>> Can you share branch and how to test?
>>>
>>>
>>> QEMU branch: https://github.com/shentok/qemu/tree/e500-flash <https://github.com/shentok/qemu/tree/e500-flash>
>>>
>>> How to test:
>>> 1. `git clone -b e500 https://github.com/shentok/buildroot.git <https://github.com/shentok/buildroot.git>`
>>> 2. `cd buildroot`
>>> 3. `make qemu_ppc_e500mc_defconfig`
>>> 4. `make`
>>> 5. `cd output/images`
>>> 6. `dd if=/dev/zero of=root.img bs=1M count=64 && dd if=rootfs.ext2 of=root.img bs=1M conv=notrunc`
>>> 7. `qemu-system-ppc -M ppce500 -cpu e500mc -m 256 -kernel uImage -append "console=ttyS0 rootwait root=/dev/mmcblk0" -device sd-card,drive=mydrive -drive id=mydrive,if=none,file=root.img,format=raw`
>>
>>Could you add an Avocado-based test?
>
>I could give it a try at least. Where would I drop the binaries?
Looks like Cederic already had a repo [1]. Let's see what I can do.
Best regards,
Bernhard
[1] https://github.com/legoater/qemu-ppc-boot/tree/main/buildroot
>
>Best regards,
>Bernhard
>>
>>> Welcome to Buildroot
>>> buildroot login:
>>
>>Regards,
>>
>>Phil.
>
next prev parent reply other threads:[~2022-12-16 14:39 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-18 21:01 [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 1/7] docs/system/ppc/ppce500: Use qemu-system-ppc64 across the board(s) Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 2/7] hw/block/pflash_cfi0{1, 2}: Error out if device length isn't a power of two Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 3/7] hw/sd/sdhci-internal: Unexport ESDHC defines Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 4/7] hw/sd/sdhci: Rename ESDHC_* defines to USDHC_* Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 5/7] hw/ppc/e500: Implement pflash handling Bernhard Beschow
2022-10-26 17:03 ` Daniel Henrique Barboza
2022-10-27 21:11 ` Philippe Mathieu-Daudé
2022-10-28 15:09 ` Daniel Henrique Barboza
2022-10-28 16:03 ` B
2022-10-28 22:42 ` Philippe Mathieu-Daudé
2022-10-29 9:29 ` Daniel Henrique Barboza
2022-10-29 11:24 ` Bernhard Beschow
2022-10-18 21:01 ` [PATCH v4 6/7] hw/sd/sdhci: Implement Freescale eSDHC device model Bernhard Beschow
2022-10-27 21:40 ` Philippe Mathieu-Daudé
2022-10-29 11:33 ` Bernhard Beschow
2022-10-29 13:04 ` Bernhard Beschow
2022-10-29 18:28 ` Bernhard Beschow
2022-10-29 23:10 ` Philippe Mathieu-Daudé
2022-10-30 11:46 ` Bernhard Beschow
2022-10-31 12:11 ` Philippe Mathieu-Daudé
2022-11-01 10:49 ` Bernhard Beschow
2022-12-16 14:38 ` Bernhard Beschow [this message]
2022-10-18 21:01 ` [PATCH v4 7/7] hw/ppc/e500: Add Freescale eSDHC to e500plat Bernhard Beschow
2022-10-26 17:11 ` Daniel Henrique Barboza
2022-10-27 21:12 ` Philippe Mathieu-Daudé
2022-10-23 8:36 ` [PATCH v4 0/7] ppc/e500: Add support for two types of flash, cleanup Bernhard Beschow
2022-10-26 17:18 ` Daniel Henrique Barboza
2022-10-26 19:51 ` B
2022-10-26 21:40 ` Daniel Henrique Barboza
2022-10-31 12:13 ` Philippe Mathieu-Daudé
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=BEA593C6-8569-44AE-8E4B-554B6D389B72@gmail.com \
--to=shentey@gmail.com \
--cc=alistair@alistair23.me \
--cc=antonynpavlov@gmail.com \
--cc=aurelien@aurel32.net \
--cc=balaton@eik.bme.hu \
--cc=bin.meng@windriver.com \
--cc=edgar.iglesias@gmail.com \
--cc=f4bug@amsat.org \
--cc=hreitz@redhat.com \
--cc=jan.kiszka@web.de \
--cc=kwolf@redhat.com \
--cc=magnus.damm@gmail.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=ysato@users.sourceforge.jp \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).