From: "Cédric Le Goater" <clg@kaod.org>
To: Andrew Jeffery <andrew@aj.id.au>, qemu-arm@nongnu.org
Cc: peter.maydell@linaro.org, joel@jms.id.au, qemu-devel@nongnu.org
Subject: Re: [PATCH 1/2] hw/sd: Configure number of slots exposed by the ASPEED SDHCI model
Date: Tue, 10 Dec 2019 09:56:28 +0100 [thread overview]
Message-ID: <c49584df-016d-9a84-b96e-6a3b2e98c6a1@kaod.org> (raw)
In-Reply-To: <11e1d38d2374a48996a3496c906db215de246583.1575938234.git-series.andrew@aj.id.au>
On 10/12/2019 01:52, Andrew Jeffery wrote:
> The AST2600 includes a second cut-down version of the SD/MMC controller
> found in the AST2500, named the eMMC controller. It's cut down in the
> sense that it only supports one slot rather than two, but it brings the
> total number of slots supported by the AST2600 to three.
>
> The existing code assumed that the SD controller always provided two
> slots. Rework the SDHCI object to expose the number of slots as a
> property to be set by the SoC configuration.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
One minor question below.
> ---
> hw/arm/aspeed.c | 2 +-
> hw/arm/aspeed_ast2600.c | 2 ++
> hw/arm/aspeed_soc.c | 3 +++
> hw/sd/aspeed_sdhci.c | 11 +++++++++--
> include/hw/sd/aspeed_sdhci.h | 1 +
> 5 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 028191ff36fc..862549b1f3a9 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -259,7 +259,7 @@ static void aspeed_board_init(MachineState *machine,
> cfg->i2c_init(bmc);
> }
>
> - for (i = 0; i < ARRAY_SIZE(bmc->soc.sdhci.slots); i++) {
> + for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
> DriveInfo *dinfo = drive_get_next(IF_SD);
> BlockBackend *blk;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 931887ac681f..931ee5aae183 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -208,6 +208,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
> sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
> TYPE_ASPEED_SDHCI);
>
> + object_property_set_int(OBJECT(&s->sdhci), 2, "num-slots", &error_abort);
OK. This defines 2 SDHCI slots for the ast2600 SoC, but
> +
> /* Init sd card slot class here so that they're under the correct parent */
> for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index f4fe243458fd..3498f55603f2 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -215,6 +215,9 @@ static void aspeed_soc_init(Object *obj)
> sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
> TYPE_ASPEED_SDHCI);
>
> + object_property_set_int(OBJECT(&s->sdhci), ASPEED_SDHCI_NUM_SLOTS,
> + "num-slots", &error_abort);
why use ASPEED_SDHCI_NUM_SLOTS here ?
C.
> /* Init sd card slot class here so that they're under the correct parent */
> for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> index cff3eb7dd21e..939d1510dedb 100644
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -13,6 +13,7 @@
> #include "qapi/error.h"
> #include "hw/irq.h"
> #include "migration/vmstate.h"
> +#include "hw/qdev-properties.h"
>
> #define ASPEED_SDHCI_INFO 0x00
> #define ASPEED_SDHCI_INFO_RESET 0x00030000
> @@ -120,14 +121,14 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
>
> /* Create input irqs for the slots */
> qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_sdhci_set_irq,
> - sdhci, NULL, ASPEED_SDHCI_NUM_SLOTS);
> + sdhci, NULL, sdhci->num_slots);
>
> sysbus_init_irq(sbd, &sdhci->irq);
> memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
> sdhci, TYPE_ASPEED_SDHCI, 0x1000);
> sysbus_init_mmio(sbd, &sdhci->iomem);
>
> - for (int i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> + for (int i = 0; i < sdhci->num_slots; ++i) {
> Object *sdhci_slot = OBJECT(&sdhci->slots[i]);
> SysBusDevice *sbd_slot = SYS_BUS_DEVICE(&sdhci->slots[i]);
>
> @@ -174,6 +175,11 @@ static const VMStateDescription vmstate_aspeed_sdhci = {
> },
> };
>
> +static Property aspeed_sdhci_properties[] = {
> + DEFINE_PROP_UINT8("num-slots", AspeedSDHCIState, num_slots, 0),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(classp);
> @@ -181,6 +187,7 @@ static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
> dc->realize = aspeed_sdhci_realize;
> dc->reset = aspeed_sdhci_reset;
> dc->vmsd = &vmstate_aspeed_sdhci;
> + dc->props = aspeed_sdhci_properties;
> }
>
> static TypeInfo aspeed_sdhci_info = {
> diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
> index dfdab4379021..dffbb46946b9 100644
> --- a/include/hw/sd/aspeed_sdhci.h
> +++ b/include/hw/sd/aspeed_sdhci.h
> @@ -24,6 +24,7 @@ typedef struct AspeedSDHCIState {
> SysBusDevice parent;
>
> SDHCIState slots[ASPEED_SDHCI_NUM_SLOTS];
> + uint8_t num_slots;
>
> MemoryRegion iomem;
> qemu_irq irq;
>
next prev parent reply other threads:[~2019-12-10 8:57 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-10 0:52 [PATCH 0/2] hw/arm: ast2600: Wire up eMMC controller Andrew Jeffery
2019-12-10 0:52 ` [PATCH 1/2] hw/sd: Configure number of slots exposed by the ASPEED SDHCI model Andrew Jeffery
2019-12-10 7:49 ` Philippe Mathieu-Daudé
2019-12-10 8:56 ` Cédric Le Goater [this message]
2019-12-10 22:17 ` Andrew Jeffery
2019-12-10 0:52 ` [PATCH 2/2] hw/arm: ast2600: Wire up the eMMC controller Andrew Jeffery
2019-12-10 12:52 ` Cédric Le Goater
2019-12-10 22:15 ` Andrew Jeffery
2019-12-10 8:53 ` [PATCH 0/2] hw/arm: ast2600: Wire up " Cédric Le Goater
2019-12-10 22:19 ` Andrew Jeffery
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=c49584df-016d-9a84-b96e-6a3b2e98c6a1@kaod.org \
--to=clg@kaod.org \
--cc=andrew@aj.id.au \
--cc=joel@jms.id.au \
--cc=peter.maydell@linaro.org \
--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
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).