On Sat, Aug 17, 2024 at 09:41:08AM +0200, Andrew Jones wrote:
> An 'msi-parent' property with a single entry and no accompanying
> '#msi-cells' property is considered the legacy definition as opposed
> to its definition after being expanded with commit 126b16e2ad98
> ("Docs: dt: add generic MSI bindings"). However, the legacy
> definition is completely compatible with the current definition and,
> since of_phandle_iterator_next() tolerates missing and present-but-
> zero *cells properties since commit e42ee61017f5 ("of: Let
> of_for_each_phandle fallback to non-negative cell_count"), there's no
> need anymore to special case the legacy definition in
> of_msi_get_domain().
>
> Indeed, special casing has turned out to be harmful, because, as of
> commit 7c025238b47a ("dt-bindings: irqchip: Describe the IMX MU block
> as a MSI controller"), MSI controller DT bindings have started
> specifying '#msi-cells' as a required property (even when the value
> must be zero) as an effort to make the bindings more explicit. But,
> since the special casing of 'msi-parent' only uses the existence of
> '#msi-cells' for its heuristic, and not whether or not it's also
> nonzero, the legacy path is not taken. Furthermore, the path to
> support the new, broader definition isn't taken either since that
> path has been restricted to the platform-msi bus.
>
> But, neither the definition of 'msi-parent' nor the definition of
> '#msi-cells' is platform-msi-specific (the platform-msi bus was just
> the first bus that needed '#msi-cells'), so remove both the special
> casing and the restriction. The code removal also requires changing
> to of_parse_phandle_with_optional_args() in order to ensure the
> legacy (but compatible) use of 'msi-parent' remains supported. This
> not only simplifies the code but also resolves an issue with PCI
> devices finding their MSI controllers on riscv, as the riscv,imsics
> binding requires '#msi-cells=<0>'.
>
> Signed-off-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> ---
> v3:
> - switch to of_for_each_phandle() to further cleanup/simplify the
> code [Rob]
> v2:
> - switch to of_parse_phandle_with_optional_args() to ensure the
> absence of #msi-cells means count=0
>
> drivers/of/irq.c | 35 ++++++++---------------------------
> 1 file changed, 8 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index c94203ce65bb..b74a3f5fc4e2 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -709,8 +709,7 @@ struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> * @np: device node for @dev
> * @token: bus type for this domain
> *
> - * Parse the msi-parent property (both the simple and the complex
> - * versions), and returns the corresponding MSI domain.
> + * Parse the msi-parent property and returns the corresponding MSI domain.
> *
> * Returns: the MSI domain for this device (or NULL on failure).
> */
> @@ -718,33 +717,15 @@ struct irq_domain *of_msi_get_domain(struct device *dev,
> struct device_node *np,
> enum irq_domain_bus_token token)
> {
> - struct device_node *msi_np;
> + struct of_phandle_iterator it;
> struct irq_domain *d;
> + int err;
>
> - /* Check for a single msi-parent property */
> - msi_np = of_parse_phandle(np, "msi-parent", 0);
> - if (msi_np && !of_property_read_bool(msi_np, "#msi-cells")) {
> - d = irq_find_matching_host(msi_np, token);
> - if (!d)
> - of_node_put(msi_np);
> - return d;
> - }
> -
> - if (token == DOMAIN_BUS_PLATFORM_MSI) {
> - /* Check for the complex msi-parent version */
> - struct of_phandle_args args;
> - int index = 0;
> -
> - while (!of_parse_phandle_with_args(np, "msi-parent",
> - "#msi-cells",
> - index, &args)) {
> - d = irq_find_matching_host(args.np, token);
> - if (d)
> - return d;
> -
> - of_node_put(args.np);
> - index++;
> - }
> + of_for_each_phandle(&it, err, np, "msi-parent", "#msi-cells", 0) {
> + d = irq_find_matching_host(it.node, token);
> + if (d)
> + return d;
> + of_node_put(it.node);
Pretty sure the iterator does this for you. I can fixup when applying.
I plan to tag for stable too.
Shout if you disagree with either of these.
Rob
