On Fri, Mar 5, 2021 at 5:55 PM Hector Martin [off-list ref] wrote:

On 06/03/2021 00.13, Andy Shevchenko wrote:

...

quoted

quoted

-       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?

...

quoted

quoted

+       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