Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210311174046.597d1951@xhacker.debian>
In-Reply-To: <20210311174046.597d1951@xhacker.debian>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 11 Mar 2021 11:57:03 +0100
Message-ID: <CAPDyKFpUM+Xpui3e8Ft2C1KpWEmR33QT-wERo33vmEJp0Grrbw@mail.gmail.com>
Subject: Re: [PATCH] mmc: sdhci: Use "mmc" directly rather than "host->mmc"
To:     Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc:     Adrian Hunter <adrian.hunter@intel.com>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 11 Mar 2021 at 10:40, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>
> Clean up the code to use the "mmc" directly instead of "host->mmc".
> If the code sits in hot code path, this clean up also brings trvial
> performance improvement. Take the sdhci_post_req() for example:
>
> before the patch:
>      ...
>      8d0:       a9be7bfd        stp     x29, x30, [sp, #-32]!
>      8d4:       910003fd        mov     x29, sp
>      8d8:       f9000bf3        str     x19, [sp, #16]
>      8dc:       f9400833        ldr     x19, [x1, #16]
>      8e0:       b9404261        ldr     w1, [x19, #64]
>      8e4:       34000161        cbz     w1, 910 <sdhci_post_req+0x50>
>      8e8:       f9424400        ldr     x0, [x0, #1160]
>      8ec:       d2800004        mov     x4, #0x0                        // #0
>      8f0:       b9401a61        ldr     w1, [x19, #24]
>      8f4:       b9403262        ldr     w2, [x19, #48]
>      8f8:       f9400000        ldr     x0, [x0]
>      8fc:       f278003f        tst     x1, #0x100
>      900:       f9401e61        ldr     x1, [x19, #56]
>      904:       1a9f17e3        cset    w3, eq  // eq = none
>      908:       11000463        add     w3, w3, #0x1
>      90c:       94000000        bl      0 <dma_unmap_sg_attrs>
>      ...
>
> After the patch:
>      ...
>      8d0:       a9be7bfd        stp     x29, x30, [sp, #-32]!
>      8d4:       910003fd        mov     x29, sp
>      8d8:       f9000bf3        str     x19, [sp, #16]
>      8dc:       f9400833        ldr     x19, [x1, #16]
>      8e0:       b9404261        ldr     w1, [x19, #64]
>      8e4:       34000141        cbz     w1, 90c <sdhci_post_req+0x4c>
>      8e8:       b9401a61        ldr     w1, [x19, #24]
>      8ec:       d2800004        mov     x4, #0x0                        // #0
>      8f0:       b9403262        ldr     w2, [x19, #48]
>      8f4:       f9400000        ldr     x0, [x0]
>      8f8:       f278003f        tst     x1, #0x100
>      8fc:       f9401e61        ldr     x1, [x19, #56]
>      900:       1a9f17e3        cset    w3, eq  // eq = none
>      904:       11000463        add     w3, w3, #0x1
>      908:       94000000        bl      0 <dma_unmap_sg_attrs>
>      ...
>
> We saved one ldr instruction: "ldr     x0, [x0, #1160]"

Nice!

Even if I think the cleanup of code makes sense alone.

>
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  drivers/mmc/host/sdhci.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>

[...]

> @@ -2489,14 +2489,14 @@ void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>         unsigned long flags;
>
>         if (enable)
> -               pm_runtime_get_noresume(host->mmc->parent);
> +               pm_runtime_get_noresume(mmc->parent);

Maybe use mmc_dev(mmc) instead? At least I think I would appreciate
consistency in the entire c-file, today it seems like both
"mmc->parent" and mmc_dev(mmc) are being used.

[...]

Kind regards
Uffe