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: <20191014061617.10296-2-daniel@0x0f.com> <20200610090421.3428945-3-daniel@0x0f.com>
In-Reply-To: <20200610090421.3428945-3-daniel@0x0f.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 10 Jun 2020 11:43:50 +0200
Message-ID: <CAK8P3a2JeH+-Naivo8B-JWxebB2ArkCLJw8_CN2Goy5bkSTBwg@mail.gmail.com>
Subject: Re: [PATCH v2 2/5] ARM: mstar: Add machine for MStar/Sigmastar
 infinity/mercury family ARMv7 SoCs
To:     Daniel Palmer <daniel@0x0f.com>
Cc:     k@japko.eu, "Bird, Timothy" <tim.bird@sony.com>,
        DTML <devicetree@vger.kernel.org>,
        Daniel Palmer <daniel@thingy.jp>,
        Rob Herring <robh+dt@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Sam Ravnborg <sam@ravnborg.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Heiko Stuebner <heiko.stuebner@theobroma-systems.com>,
        Maxime Ripard <mripard@kernel.org>,
        Lubomir Rintel <lkundrak@v3.sk>,
        Stephan Gerhold <stephan@gerhold.net>,
        Mark Brown <broonie@kernel.org>, allen <allen.chen@ite.com.tw>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jonathan Corbet <corbet@lwn.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mike Rapoport <rppt@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Doug Anderson <armlinux@m.disordat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Gregory Fong <gregory.0xf0@gmail.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Christian Lamparter <chunkeey@gmail.com>,
        Nathan Huckleberry <nhuck15@gmail.com>,
        =?UTF-8?Q?Andreas_F=C3=A4rber?= <afaerber@suse.de>,
        Ard Biesheuvel <ardb@kernel.org>,
        Marc Zyngier <maz@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 10, 2020 at 11:08 AM Daniel Palmer <daniel@0x0f.com> wrote:

> +/*
> + * This may need locking to deal with situations where an interrupt
> + * happens while we are in here and mb() gets called by the interrupt handler.
> + */

I would suspect that you don't need locking here, and locking would likely
make it worse.

From what I can tell, an interrupt happening anywhere inside of mstarv7_mb()
would still result in the function completing (as miu_status still has the
MSTARV7_L3BRIDGE_STATUS_DONE bit set) and nothing that could
happen inside the irq would make the barrier weaker, only stronger.

> +static void mstarv7_mb(void)
> +{
> +       /* toggle the flush miu pipe fire bit */
> +       writel_relaxed(0, miu_flush);
> +       writel_relaxed(MSTARV7_L3BRIDGE_FLUSH_TRIGGER, miu_flush);
> +       while (!(readl_relaxed(miu_status) & MSTARV7_L3BRIDGE_STATUS_DONE)) {
> +               /* wait for flush to complete */
> +       }
> +}

This is a heavy memory barrier indeed.

The use of _relaxed() accessors is normally a bad idea and should be
avoided, but
this is one of the places where it is necessary because the normal
writel()/readl()
would recurse into arm_heavy_barrier(). Can you add a comment explaining this
for the next reviewer?

> +static void __init mstarv7_barriers_init(void)
> +{
> +       miu_flush = ioremap(MSTARV7_L3BRIDGE_FLUSH, sizeof(*miu_flush));
> +       miu_status = ioremap(MSTARV7_L3BRIDGE_STATUS, sizeof(*miu_status));
> +       soc_mb = mstarv7_mb;
> +}

Hardcoding physical addresses is generally considered bad style,
even if you know the address in advance. Can you change this to get
the address of the L3BRIDGE from DT instead and just hardcode the
offsets? Note that they are in the same physical page, so you only
need a single of_iomap().

> +static void __init mstarv7_init(void)
> +{
> +       mstarv7_barriers_init();
> +}

I think you can fold this into a single function.

       Arnd