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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 71FAAC433F5 for ; Wed, 17 Nov 2021 17:52:25 +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 047F961929 for ; Wed, 17 Nov 2021 17:52:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 047F961929 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:60386 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mnP6e-0005S2-8C for qemu-devel@archiver.kernel.org; Wed, 17 Nov 2021 12:52:24 -0500 Received: from eggs.gnu.org ([209.51.188.92]:51608) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mnP4x-0003Li-Oo for qemu-devel@nongnu.org; Wed, 17 Nov 2021 12:50:39 -0500 Received: from [2a00:1450:4864:20::131] (port=37700 helo=mail-lf1-x131.google.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mnP4v-0002hu-BE for qemu-devel@nongnu.org; Wed, 17 Nov 2021 12:50:39 -0500 Received: by mail-lf1-x131.google.com with SMTP id c32so12459678lfv.4 for ; Wed, 17 Nov 2021 09:50:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HoSgA3Rjjgl4hCl3T+4qtK0fiyAYACj89eTCuIOkpIM=; b=mWINr3R1u5WQRz1eUNvkk8KXzKxhtbdtlgucqMo1YWhdrt2dFVVPYYBK2z4lC/n5xs aiHGXSNHdBF4kYYPReEpzt9WWgxiFo2TZpdUpj/YhOxeY1yeR7kvYSTSGmdgFhwh0Qaz 0omW+ON0K61U/A1vOkmIrU/jo9fvFTXbk4LHZuNjMDnR9AWt6cVfkcPgI/hDqnDCNqz6 2URACEu34K6X1KlLDrrFjaVQrqCmMdRYABbMAAy9du10jrfQhZPQ6IZpMKeOHkE9cNnd f0J4BuGWeFNW8PV66F9HwVODWHnixKiA0Xple3RMtTlXc9u3Iy6rxtdDL5B3EOzXZKjr 0wnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HoSgA3Rjjgl4hCl3T+4qtK0fiyAYACj89eTCuIOkpIM=; b=6rmm7N30Ux5/kI9yBABGxuVS67umYjSmRX5/hVA772rbradoOTiC6OeQ/HHU3QFsaQ 2VBoQfxdhRQKixIeWgDr1t7PqU4pvDQKFTE1Nq9cjhKICutcM/VdJqj5XxOxtmL7vV/p xkeOqB1JmifKpHa3SxjgaBquLQIbqSKsvlQ8/h7MwY6opUuTT9P+oQmh1jxS4XZf5cRU qIg9pnY2tdd/m8d6CThlVCCfnymNcI/P+8XaLDnNR/2Bi1ZdOCUG26iphLWdmQaBOTzp yh0CJDUvU8piwyg0qoAnrJBHf58/LNwPbUpIEDa4PrqE23lOc/bB3mxJX0fk4lviOAcP aCVQ== X-Gm-Message-State: AOAM533cAUinxN24HYyWDkPrnVrTsJzumucm/KH4brS34WGU9RZbwTi2 UtLNtWK35eu6fKWN118OHABI6HBaXhO8MMvWKDHh1g== X-Google-Smtp-Source: ABdhPJwzMeciXMd/5U1S5KWEiHghMOh2vJpRefedoi9UUB/bXhlmt0XxWIjjb7HErnADpspotHw43vg2/cW1q1LQ7pM= X-Received: by 2002:a05:6512:368c:: with SMTP id d12mr16625478lfs.538.1637171434629; Wed, 17 Nov 2021 09:50:34 -0800 (PST) MIME-Version: 1.0 References: <20211117163409.3587705-1-armbru@redhat.com> <20211117163409.3587705-4-armbru@redhat.com> In-Reply-To: From: Hao Wu Date: Wed, 17 Nov 2021 09:50:13 -0800 Message-ID: Subject: Re: [PATCH v2 03/13] hw/arm/npcm7xx_boards: Replace drive_get_next() by drive_get() To: Havard Skinnemoen Cc: Markus Armbruster , qemu-devel@nongnu.org, qemu-block@nongnu.org, Tyrone Ting , Peter Maydell , qemu-arm@nongnu.org Content-Type: multipart/alternative; boundary="000000000000cccdb605d0ffadee" X-Host-Lookup-Failed: Reverse DNS lookup failed for 2a00:1450:4864:20::131 (failed) Received-SPF: pass client-ip=2a00:1450:4864:20::131; envelope-from=wuhaotsh@google.com; helo=mail-lf1-x131.google.com X-Spam_score_int: -167 X-Spam_score: -16.8 X-Spam_bar: ---------------- X-Spam_report: (-16.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_MED=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, ENV_AND_HDR_SPF_MATCH=-0.5, HTML_MESSAGE=0.001, PDS_HP_HELO_NORDNS=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5, USER_IN_DEF_SPF_WL=-7.5 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000cccdb605d0ffadee Content-Type: text/plain; charset="UTF-8" Yes, there's SD and MMC buses. It looks like the current code only supports mmc ("soc->mmc.sdhci") but not the sd ("soc->sd.sdhci"). It's probably good to make the bus number a parameter as well and use them to distinguish. We might need a separate patch to do that. On Wed, Nov 17, 2021 at 8:54 AM Havard Skinnemoen wrote: > On Wed, Nov 17, 2021 at 8:34 AM Markus Armbruster > wrote: > > > > drive_get_next() is basically a bad idea. It returns the "next" block > > backend of a certain interface type. "Next" means bus=0,unit=N, where > > subsequent calls count N up from zero, per interface type. > > > > This lets you define unit numbers implicitly by execution order. If the > > order changes, or new calls appear "in the middle", unit numbers change. > > ABI break. Hard to spot in review. > > > > Machine "quanta-gbs-bmc" connects just one backend with > > drive_get_next(), but with a helper function. Change it to use > > drive_get() directly. This makes the unit numbers explicit in the > > code. > > > > Cc: Havard Skinnemoen > > Cc: Tyrone Ting > > Cc: Peter Maydell > > Cc: qemu-arm@nongnu.org > > Signed-off-by: Markus Armbruster > > --- > > hw/arm/npcm7xx_boards.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c > > index dec7d16ae5..d8a49e4e85 100644 > > --- a/hw/arm/npcm7xx_boards.c > > +++ b/hw/arm/npcm7xx_boards.c > > @@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, > MemoryRegion *dram) > > &error_abort); > > } > > > > -static void sdhci_attach_drive(SDHCIState *sdhci) > > +static void sdhci_attach_drive(SDHCIState *sdhci, int unit) > > { > > - DriveInfo *di = drive_get_next(IF_SD); > > + DriveInfo *di = drive_get(IF_SD, 0, unit); > > +Hao Wu IIRC the chip has separate SD and eMMC buses. Would it make > sense to take the bus number as a parameter as well? Is bus 0 the > right one to use in this case? > > The existing code always uses bus 0, so this is an improvement either way. > > Reviewed-by: Havard Skinnemoen > > > BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL; > > > > BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus"); > > @@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine) > > drive_get(IF_MTD, 0, 0)); > > > > quanta_gbs_i2c_init(soc); > > - sdhci_attach_drive(&soc->mmc.sdhci); > > + sdhci_attach_drive(&soc->mmc.sdhci, 0); > > npcm7xx_load_kernel(machine, soc); > > } > > > > -- > > 2.31.1 > > > --000000000000cccdb605d0ffadee Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Yes, there's SD and MMC buses. It looks like the curre= nt code only supports mmc ("soc->mmc.sdhci") but not the sd (&= quot;soc->sd.sdhci").

It's probably good to = make the bus number a parameter as well and use them to distinguish. We mig= ht need a separate patch to do that.

On Wed, Nov 17, 2021 at 8:54 AM H= avard Skinnemoen <hskinnemoen@= google.com> wrote:
On Wed, Nov 17, 2021 at 8:34 AM Markus Armbruster <armbru@redhat.com> wrot= e:
>
> drive_get_next() is basically a bad idea.=C2=A0 It returns the "n= ext" block
> backend of a certain interface type.=C2=A0 "Next" means bus= =3D0,unit=3DN, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.=C2=A0= If the
> order changes, or new calls appear "in the middle", unit num= bers change.
> ABI break.=C2=A0 Hard to spot in review.
>
> Machine "quanta-gbs-bmc" connects just one backend with
> drive_get_next(), but with a helper function.=C2=A0 Change it to use > drive_get() directly.=C2=A0 This makes the unit numbers explicit in th= e
> code.
>
> Cc: Havard Skinnemoen <hskinnemoen@google.com>
> Cc: Tyrone Ting <kfting@nuvoton.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@= nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>=C2=A0 hw/arm/npcm7xx_boards.c | 6 +++---
>=C2=A0 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index dec7d16ae5..d8a49e4e85 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, = MemoryRegion *dram)
>=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&error_abort);
>=C2=A0 }
>
> -static void sdhci_attach_drive(SDHCIState *sdhci)
> +static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveInfo *di =3D drive_get_next(IF_SD);<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 DriveInfo *di =3D drive_get(IF_SD, 0, uni= t);

+Hao Wu IIRC the chip has separate SD and eMMC buses. Would it make
sense to take the bus number as a parameter as well? Is bus 0 the
right one to use in this case?

The existing code always uses bus 0, so this is an improvement either way.<= br>
Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>

>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BlockBackend *blk =3D di ? blk_by_le= gacy_dinfo(di) : NULL;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BusState *bus =3D qdev_get_child_bus= (DEVICE(sdhci), "sd-bus");
> @@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)=
>=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 drive_get(IF_MTD, 0, 0));
>
>=C2=A0 =C2=A0 =C2=A0 quanta_gbs_i2c_init(soc);
> -=C2=A0 =C2=A0 sdhci_attach_drive(&soc->mmc.sdhci);
> +=C2=A0 =C2=A0 sdhci_attach_drive(&soc->mmc.sdhci, 0);
>=C2=A0 =C2=A0 =C2=A0 npcm7xx_load_kernel(machine, soc);
>=C2=A0 }
>
> --
> 2.31.1
>
--000000000000cccdb605d0ffadee--