Message-ID: <20210308211306.GA2920998@robh.at.kernel.org>
Date:   Mon, 8 Mar 2021 14:13:06 -0700
From:   Rob Herring <robh@...nel.org>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Hector Martin <marcan@...can.st>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>, Olof Johansson <olof@...om.net>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Tony Lindgren <tony@...mide.com>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Stan Skowronek <stan@...ellium.com>,
        Alexander Graf <graf@...zon.com>,
        Will Deacon <will@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Catalin Marinas <catalin.marinas@....com>,
        Christoph Hellwig <hch@...radead.org>,
        "David S. Miller" <davem@...emloft.net>,
        DTML <devicetree@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
        "open list:GENERIC INCLUDE/ASM HEADER FILES" 
        <linux-arch@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFT PATCH v3 12/27] of/address: Add infrastructure to declare
 MMIO as non-posted

On Mon, Mar 08, 2021 at 09:29:54PM +0100, Arnd Bergmann wrote:
> On Mon, Mar 8, 2021 at 4:56 PM Rob Herring <robh@...nel.org> wrote:
> > On Fri, Mar 5, 2021 at 2:17 PM Arnd Bergmann <arnd@...nel.org> wrote:
> > > On Fri, Mar 5, 2021 at 7:18 PM Hector Martin <marcan@...can.st> wrote:
> >
> > > > > What's the code path using these functions on the M1 where we need to
> > > > > return 'posted'? It's just downstream PCI mappings (PCI memory space),
> > > > > right? Those would never hit these paths because they don't have a DT
> > > > > node or if they do the memory space is not part of it. So can't the
> > > > > check just be:
> > > > >
> > > > > bool of_mmio_is_nonposted(struct device_node *np)
> > > > > {
> > > > >      return np && of_machine_is_compatible("apple,arm-platform");
> > > > > }
> > > >
> > > > Yes; the implementation was trying to be generic, but AIUI we don't need
> > > > this on M1 because the PCI mappings don't go through this codepath, and
> > > > nothing else needs posted mode. My first hack was something not too
> > > > unlike this, then I was going to get rid of apple,arm-platform and just
> > > > have this be a generic mechanism with the properties, but then we added
> > > > the optimization to not do the lookups on other platforms, and now we're
> > > > coming full circle... :-)
> > >
> > > I never liked the idea of having a list of platforms that need a
> > > special hack, please let's not go back to that.
> >
> > I'm a fan of generic solutions as much as anyone, but not when there's
> > a single user. Yes, there could be more, but we haven't seen any yet
> > and Apple seems to have a knack for doing special things. I'm pretty
> > sure posted vs. non-posted has been a possibility with AXI buses from
> > the start, so it's not like this is a new thing we're going to see
> > frequently on new platforms.
> 
> Ok, but if we make it a platform specific bit, I would prefer not
> to do the IORESOURCE_MEM_NONPOSTED flag either but
> instead keep the logic in the device drivers that call ioremap().

That seems like an orthogonal decision to me.

> This is obviously more work for the drivers, but at least it keeps
> the common code free of the hack while also allowing drivers to
> use ioremap_np() intentionally on other platforms.

I don't agree. The problem is within the interconnect. The device and 
its driver are unaware of this.

The other idea I had was doing a compatible other than 'simple-bus' for 
the bus node which could imply non-posted io and any other quirks in 
Apple's bus implementation. However, something different there means 
updates in lots of places (schemas, dtc checks, etc.) unless we kept 
'simple-bus' as a fallback.

Let's just stick with 'nonposted-mmio', but drop 'posted-mmio'. I'd 
rather know if and when we need 'posted-mmio'. It does need to be added 
to the DT spec[1] and schema[2] though (GH PRs are fine for both).

> > The other situation I worry about here is another arch has implicitly
> > defaulted to non-posted instead of posted. It could just be non-posted
> > was what worked everywhere and Linux couldn't distinguish. Now someone
> > sees we have this new posted vs. non-posted handling and can optimize
> > some mappings on their platform and we have to have per arch defaults
> > (like 'dma-coherent' now).
> 
> I think one of the dark secrets of MMIO is that a lot of drivers
> get the posted behavior wrong by assuming that a writel() before
> a spin_unlock() is protected by that unlock. This may in fact work
> on many architectures but is broken on PCI and on local devices
> for ARM.
> 
> Having a properly working (on non-PCI) ioremap_np() interface
> would be nice here, as it could be used to document when drivers
> rely on non-posted behavior, and cause the ioremap to fail when
> running on architectures that don't support nonposted maps.

Good to know.

Rob

[1] https://github.com/devicetree-org/devicetree-specification
[2] https://github.com/devicetree-org/dt-schema

Powered by blists - more mailing lists