Re: [RFT PATCH v3 12/27] of/address: Add infrastructure to declare MMIO as non-posted

[Rob Herring]

On Thu, Mar 4, 2021 at 3:40 PM Hector Martin <marcan@xxxxxxxxx> wrote:
>
> This implements the 'nonposted-mmio' and 'posted-mmio' boolean
> properties. Placing these properties in a bus marks all child devices as
> requiring non-posted or posted MMIO mappings. If no such properties are
> found, the default is posted MMIO.

I'm still a little hesitant to add these properties and having some
default. I worry about a similar situation as 'dma-coherent' where the
assumed default on non-coherent on Arm doesn't work for PowerPC which
defaults coherent. More below on this.

> of_mmio_is_nonposted() performs the tree walking to determine if a given
> device has requested non-posted MMIO.
>
> of_address_to_resource() uses this to set the IORESOURCE_MEM_NONPOSTED
> flag on resources that require non-posted MMIO.
>
> of_iomap() and of_io_request_and_map() then use this flag to pick the
> correct ioremap() variant.
>
> This mechanism is currently restricted to Apple ARM platforms, as an
> optimization.
>
> Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
> ---
>  drivers/of/address.c       | 72 ++++++++++++++++++++++++++++++++++++--
>  include/linux/of_address.h |  1 +
>  2 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 73ddf2540f3f..6114dceb1ba6 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -847,6 +847,9 @@ static int __of_address_to_resource(struct device_node *dev,
>                 return -EINVAL;
>         memset(r, 0, sizeof(struct resource));
>
> +       if (of_mmio_is_nonposted(dev))
> +               flags |= IORESOURCE_MEM_NONPOSTED;
> +
>         r->start = taddr;
>         r->end = taddr + size - 1;
>         r->flags = flags;
> @@ -896,7 +899,10 @@ void __iomem *of_iomap(struct device_node *np, int index)
>         if (of_address_to_resource(np, index, &res))
>                 return NULL;
>
> -       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 and the devm variants all scream for a ioremap_extended()
function. IOW, it would be better if the ioremap flavor was a
parameter. Unless we could implement that just for arm64 first, that's
a lot of refactoring...

>  }
>  EXPORT_SYMBOL(of_iomap);
>
> @@ -928,7 +934,11 @@ void __iomem *of_io_request_and_map(struct device_node *np, int index,
>         if (!request_mem_region(res.start, resource_size(&res), name))
>                 return IOMEM_ERR_PTR(-EBUSY);
>
> -       mem = ioremap(res.start, resource_size(&res));
> +       if (res.flags & IORESOURCE_MEM_NONPOSTED)
> +               mem = ioremap_np(res.start, resource_size(&res));
> +       else
> +               mem = ioremap(res.start, resource_size(&res));
> +
>         if (!mem) {
>                 release_mem_region(res.start, resource_size(&res));
>                 return IOMEM_ERR_PTR(-ENOMEM);
> @@ -1094,3 +1104,61 @@ bool of_dma_is_coherent(struct device_node *np)
>         return false;
>  }
>  EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> +
> +static bool of_nonposted_mmio_quirk(void)
> +{
> +       if (IS_ENABLED(CONFIG_ARCH_APPLE)) {
> +               /* To save cycles, we cache the result for global "Apple ARM" setting */
> +               static int quirk_state = -1;
> +
> +               /* Make quirk cached */
> +               if (quirk_state < 0)
> +                       quirk_state = of_machine_is_compatible("apple,arm-platform");
> +               return !!quirk_state;
> +       }
> +       return false;
> +}
> +
> +/**
> + * of_mmio_is_nonposted - Check if device uses non-posted MMIO
> + * @np:        device node
> + *
> + * Returns true if the "nonposted-mmio" property was found for
> + * the device's bus or a parent. "posted-mmio" has the opposite
> + * effect, terminating recursion and overriding any
> + * "nonposted-mmio" properties in parent buses.
> + *
> + * Recursion terminates if reach a non-translatable boundary
> + * (a node without a 'ranges' property).
> + *
> + * This is currently only enabled on Apple ARM devices, as an
> + * optimization.
> + */
> +bool of_mmio_is_nonposted(struct device_node *np)
> +{
> +       struct device_node *node;
> +       struct device_node *parent;
> +
> +       if (!of_nonposted_mmio_quirk())
> +               return false;
> +
> +       node = of_get_parent(np);
> +
> +       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;
> +       }
> +
> +       of_node_put(node);
> +       return false;

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");
}

Note in theory we could use 'assigned-addresses' with PCI, but that's
pretty much never the case with FDT. If we did, we could detect the
device node is a PCI device in that case.

Rob