linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Hector Martin <marcan@marcan.st>
Cc: linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Marc Zyngier <maz@kernel.org>, Rob Herring <robh@kernel.org>,
	Arnd Bergmann <arnd@kernel.org>, Olof Johansson <olof@lixom.net>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	Tony Lindgren <tony@atomide.com>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	Alexander Graf <graf@amazon.com>, Will Deacon <will@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Christoph Hellwig <hch@infradead.org>,
	"David S. Miller" <davem@davemloft.net>,
	devicetree <devicetree@vger.kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Linux Documentation List <linux-doc@vger.kernel.org>,
	Linux Samsung SOC <linux-samsung-soc@vger.kernel.org>,
	Linux-Arch <linux-arch@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFT PATCH v3 12/27] of/address: Add infrastructure to declare MMIO as non-posted
Date: Fri, 5 Mar 2021 18:08:27 +0200	[thread overview]
Message-ID: <CAHp75Vd6adVM94G1vCrQcZoegQFWHbK14YRRuBTQZwrM5CV2jQ@mail.gmail.com> (raw)
In-Reply-To: <04ea35d6-cd7d-d6de-75ae-59b1e0c77f04@marcan.st>

On Fri, Mar 5, 2021 at 5:55 PM Hector Martin <marcan@marcan.st> wrote:
> On 06/03/2021 00.13, Andy Shevchenko wrote:

...

> >> -       return ioremap(res.start, resource_size(&res));
> >> +       if (res.flags & IORESOURCE_MEM_NONPOSTED)
> >> +               return ioremap_np(res.start, resource_size(&res));
> >> +       else
> >> +               return ioremap(res.start, resource_size(&res));
> >
> > This doesn't sound right. Why _np is so exceptional? Why don't we have
> > other flavours (it also rings a bell to my previous comment that the
> > flag in ioresource is not in the right place)?
>
> This is different from other variants, because until now *drivers* have
> made the choice of what ioremap mode to use based on device requirements
> (which means ioremap() 99% of the time, and then framebuffers and other
> memory-ish things such use something else). Now we have a *SoC fabric*
> that is calling the shots on what ioremap mode we have to use - and
> *every* non-PCIe driver needs to use ioremap_np() on these SoCs, or they
> break. So it seems a lot cleaner to make the choice for drivers here to
> upgrade ioremap() to ioremap_np() for SoCs that need it.

Yes, that is a good idea. Once we discussed x86 and _uc cases and
actually on x86 it makes a lot of sense to have ioremap() ==
ioremap_uc(). Can't be this a similar case here?
Arnd, what do you think of actually providing an ioremap() as some
kind of "best for the architecture the code is running on"?

Otherwise if the same driver happens to be needed on different
architectures, oops, ifdeffery or simple conditionals over the code is
really not the best way to solve it.

> If we don't do something like this here or in otherwise common code,
> we'd have to have an open-coded "if apple then ioremap_np, else ioremap"
> in every driver that runs on-die devices on these SoCs, even ones that
> are otherwise standard and need few or no Apple-specific quirks.

Exactly! But what about architectures where _uc is that one? So, why
does your patch only take part of _np case?
(Hint we have x86 Device Tree based platforms)

> We're still going to have to patch some drivers to use managed APIs that
> can properly hit this conditional (like I did for samsung_tty) in cases
> where they currently don't, but that's a lot cleaner than an open-coded
> conditional, I think (and often comes with other benefits anyway).
>
> Note that wholesale making ioremap() behave like ioremap_np() at the
> arch level as as SoC quirk is not an option - for extenal PCIe devices,
> we still need to use ioremap(). We tried this approach initially but it
> doesn't work. Hence we arrived at this solution which describes the
> required mode in the devicetree, at the bus level (which makes sense,
> since that represents the fabric), and then these wrappers can use that
> information, carried over via the bit in struct device, to pick the
> right ioremap mode.
>
> It doesn't really make sense to include the other variants here, because
> _np is strictly stronger than the default. Downgrading ioremap to any
> other variant would break most drivers, badly. However, upgrading to
> ioremap_np() is always correct (if possibly slower), on platforms where
> it is allowed by the bus. In fact, I bet that on many systems nGnRE
> already behaves like nGnRnE anyway. I don't know why Apple didn't just
> allow nGnRE mappings to work (behaving like nGnRnE) instead of making
> them explode, which is the whole reason we have to do this.

Yep, and why not to make ioremap() == ioremap_nc() on architecture
that requires it?
Can it be detected at run time?

...

> >> +       while (node) {
> >> +               if (!of_property_read_bool(node, "ranges")) {
> >> +                       break;
> >> +               } else if (of_property_read_bool(node, "nonposted-mmio")) {
> >> +                       of_node_put(node);
> >> +                       return true;
> >> +               } else if (of_property_read_bool(node, "posted-mmio")) {
> >> +                       break;
> >> +               }
> >> +               parent = of_get_parent(node);
> >> +               of_node_put(node);
> >> +               node = parent;
> >> +       }
> >
> > I believe above can be slightly optimized. Don't we have helpers to
> > traverse to all parents?
>
> Keep in mind the logic here is that it stops on the first instance of
> either property, and does not traverse non-translatable boundaries. Are
> there helpers that can implement this kind of complex logic? It's not a
> simple recursive property lookup.

I am aware of what it does and I believe if we don't have such a
helper yet we may introduce it and maybe even existing users of
something similar can utilize it.

-- 
With Best Regards,
Andy Shevchenko

  reply	other threads:[~2021-03-05 16:09 UTC|newest]

Thread overview: 136+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-04 21:38 [RFT PATCH v3 00/27] Apple M1 SoC platform bring-up Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 01/27] arm64: Cope with CPUs stuck in VHE mode Hector Martin
2021-03-24 18:05   ` Will Deacon
2021-03-24 20:00     ` Marc Zyngier
2021-03-26  7:54       ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 02/27] dt-bindings: vendor-prefixes: Add apple prefix Hector Martin
2021-03-08 20:26   ` Rob Herring
2021-03-04 21:38 ` [RFT PATCH v3 03/27] dt-bindings: arm: apple: Add bindings for Apple ARM platforms Hector Martin
2021-03-05 10:16   ` Linus Walleij
2021-03-08 20:27   ` Rob Herring
2021-03-04 21:38 ` [RFT PATCH v3 04/27] dt-bindings: arm: cpus: Add apple,firestorm & icestorm compatibles Hector Martin
2021-03-08 20:27   ` Rob Herring
2021-03-04 21:38 ` [RFT PATCH v3 05/27] arm64: cputype: Add CPU implementor & types for the Apple M1 cores Hector Martin
2021-03-24 18:13   ` Will Deacon
2021-03-04 21:38 ` [RFT PATCH v3 06/27] dt-bindings: timer: arm,arch_timer: Add interrupt-names support Hector Martin
2021-03-05 10:18   ` Linus Walleij
2021-03-08 11:12   ` Marc Zyngier
2021-03-08 17:14   ` Tony Lindgren
2021-03-08 20:38   ` Rob Herring
2021-03-08 22:42     ` Marc Zyngier
2021-03-09 16:11       ` Rob Herring
2021-03-09 20:28         ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 07/27] arm64: arch_timer: implement support for interrupt-names Hector Martin
2021-03-05 10:19   ` Linus Walleij
2021-03-08 11:13   ` Marc Zyngier
2021-03-04 21:38 ` [RFT PATCH v3 08/27] asm-generic/io.h: Add a non-posted variant of ioremap() Hector Martin
2021-03-05 14:45   ` Andy Shevchenko
2021-03-05 15:19     ` Hector Martin
2021-03-08 11:20   ` Marc Zyngier
2021-03-24 18:12   ` Will Deacon
2021-03-24 19:09     ` Arnd Bergmann
2021-03-25 14:07       ` Hector Martin
2021-03-25 14:49         ` Will Deacon
2021-03-04 21:38 ` [RFT PATCH v3 09/27] docs: driver-api: device-io: Document I/O access functions Hector Martin
2021-03-05 10:22   ` Linus Walleij
2021-03-04 21:38 ` [RFT PATCH v3 10/27] docs: driver-api: device-io: Document ioremap() variants & access funcs Hector Martin
2021-03-05 10:25   ` Linus Walleij
2021-03-05 15:09     ` Andy Shevchenko
2021-03-05 15:51       ` Arnd Bergmann
2021-03-09 20:29         ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 11/27] arm64: Implement ioremap_np() to map MMIO as nGnRnE Hector Martin
2021-03-08 11:22   ` Marc Zyngier
2021-03-24 18:18   ` Will Deacon
2021-03-04 21:38 ` [RFT PATCH v3 12/27] of/address: Add infrastructure to declare MMIO as non-posted Hector Martin
2021-03-05 10:28   ` Linus Walleij
2021-03-05 15:13   ` Andy Shevchenko
2021-03-05 15:55     ` Hector Martin
2021-03-05 16:08       ` Andy Shevchenko [this message]
2021-03-05 16:43         ` Arnd Bergmann
2021-03-05 17:19           ` Hector Martin
2021-03-05 16:05     ` Rob Herring
2021-03-05 17:39   ` Rob Herring
2021-03-05 18:18     ` Hector Martin
2021-03-05 21:17       ` Arnd Bergmann
2021-03-08 15:56         ` Rob Herring
2021-03-08 20:29           ` Arnd Bergmann
2021-03-08 21:13             ` Rob Herring
2021-03-08 21:56               ` Arnd Bergmann
2021-03-09 15:48                 ` Rob Herring
2021-03-09 20:23                   ` Hector Martin
2021-03-09 22:06                     ` Rob Herring
2021-03-10  8:26                       ` Hector Martin
2021-03-10 17:01                         ` Rob Herring
2021-03-11  9:12                           ` Arnd Bergmann
2021-03-11 12:11                             ` Hector Martin
2021-03-11 13:35                               ` Arnd Bergmann
2021-03-11 16:07                             ` Rob Herring
2021-03-11 16:48                               ` Arnd Bergmann
2021-03-11 18:10                                 ` Rob Herring
2021-03-12 10:20                                   ` Arnd Bergmann
2021-03-09 11:14               ` Linus Walleij
2021-03-09 12:41                 ` Arnd Bergmann
2021-03-09 15:40                   ` Linus Walleij
2021-03-04 21:38 ` [RFT PATCH v3 13/27] arm64: Add Apple vendor-specific system registers Hector Martin
2021-03-24 18:38   ` Will Deacon
2021-03-24 18:59     ` Mark Rutland
2021-03-24 19:04       ` Will Deacon
2021-03-26  6:23         ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 14/27] arm64: move ICH_ sysreg bits from arm-gic-v3.h to sysreg.h Hector Martin
2021-03-08 11:39   ` Marc Zyngier
2021-03-24 18:23   ` Will Deacon
2021-03-04 21:38 ` [RFT PATCH v3 15/27] dt-bindings: interrupt-controller: Add DT bindings for apple-aic Hector Martin
2021-03-08 21:16   ` Rob Herring
2021-03-04 21:38 ` [RFT PATCH v3 16/27] irqchip/apple-aic: Add support for the Apple Interrupt Controller Hector Martin
2021-03-05 15:05   ` Andy Shevchenko
2021-03-08 11:50     ` Marc Zyngier
2021-03-08 12:02       ` Andy Shevchenko
2021-03-26 13:40     ` Hector Martin
2021-03-08 13:31   ` Marc Zyngier
2021-03-26  7:57     ` Hector Martin
2021-03-24 19:57   ` Will Deacon
2021-03-26  8:58     ` Hector Martin
2021-03-29 12:04       ` Will Deacon
2021-04-01 13:16         ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 17/27] arm64: Kconfig: Introduce CONFIG_ARCH_APPLE Hector Martin
2021-03-08 15:35   ` Marc Zyngier
2021-03-09 20:30     ` Hector Martin
2021-03-04 21:38 ` [RFT PATCH v3 18/27] tty: serial: samsung_tty: Separate S3C64XX ops structure Hector Martin
2021-03-05 10:30   ` Krzysztof Kozlowski
2021-03-04 21:38 ` [RFT PATCH v3 19/27] tty: serial: samsung_tty: Add ucon_mask parameter Hector Martin
2021-03-05 10:34   ` Krzysztof Kozlowski
2021-03-04 21:38 ` [RFT PATCH v3 20/27] tty: serial: samsung_tty: Add s3c24xx_port_type Hector Martin
2021-03-05 10:49   ` Krzysztof Kozlowski
2021-03-04 21:38 ` [RFT PATCH v3 21/27] tty: serial: samsung_tty: IRQ rework Hector Martin
2021-03-05 10:51   ` Krzysztof Kozlowski
2021-03-05 15:17   ` Andy Shevchenko
2021-03-05 16:16     ` Hector Martin
2021-03-05 16:20       ` Andy Shevchenko
2021-03-05 16:29         ` Hector Martin
2021-03-07 11:34           ` Krzysztof Kozlowski
2021-03-07 16:01             ` Arnd Bergmann
2021-03-07 19:51               ` Krzysztof Kozlowski
2021-03-04 21:38 ` [RFT PATCH v3 22/27] tty: serial: samsung_tty: Use devm_ioremap_resource Hector Martin
2021-03-05 10:54   ` Krzysztof Kozlowski
2021-03-05 15:19     ` Andy Shevchenko
2021-03-04 21:38 ` [RFT PATCH v3 23/27] dt-bindings: serial: samsung: Add apple,s5l-uart compatible Hector Martin
2021-03-08 21:17   ` Rob Herring
2021-03-04 21:38 ` [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs Hector Martin
2021-03-05 10:58   ` Krzysztof Kozlowski
2021-03-05 15:28   ` Andy Shevchenko
2021-03-05 17:04     ` Hector Martin
2021-03-07 11:40       ` Krzysztof Kozlowski
2021-03-04 21:39 ` [RFT PATCH v3 25/27] tty: serial: samsung_tty: Add earlycon " Hector Martin
2021-03-05 10:55   ` Krzysztof Kozlowski
2021-03-10 23:11   ` Linus Walleij
2021-03-04 21:39 ` [RFT PATCH v3 26/27] dt-bindings: display: Add apple,simple-framebuffer Hector Martin
2021-03-08 21:18   ` Rob Herring
2021-03-09 16:37   ` Linus Walleij
2021-03-09 20:35     ` Hector Martin
2021-03-04 21:39 ` [RFT PATCH v3 27/27] arm64: apple: Add initial Apple Mac mini (M1, 2020) devicetree Hector Martin
2021-03-05 11:03   ` Krzysztof Kozlowski
2021-03-05 11:14     ` Hector Martin
2021-03-05 11:45       ` Krzysztof Kozlowski
2021-03-05 15:59       ` Mark Kettenis
2021-03-05 16:50         ` Hector Martin
2021-03-05 10:11 ` [RFT PATCH v3 00/27] Apple M1 SoC platform bring-up Hector Martin

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=CAHp75Vd6adVM94G1vCrQcZoegQFWHbK14YRRuBTQZwrM5CV2jQ@mail.gmail.com \
    --to=andy.shevchenko@gmail.com \
    --cc=arnd@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=graf@amazon.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=krzk@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=marcan@marcan.st \
    --cc=mark.kettenis@xs4all.nl \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mohamed.mediouni@caramail.com \
    --cc=olof@lixom.net \
    --cc=robh@kernel.org \
    --cc=stan@corellium.com \
    --cc=tony@atomide.com \
    --cc=will@kernel.org \
    --subject='Re: [RFT PATCH v3 12/27] of/address: Add infrastructure to declare MMIO as non-posted' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox