Re: [PATCH v4 1/5] of_address: Add bus type match for pci ranges parser

[Rob Herring]

On Fri, Aug 14, 2020 at 12:21 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi all,
>
> On 2020-07-28 16:36, Jiaxun Yang wrote:
> > So the parser can be used to parse range property of ISA bus.
> >
> > As they're all using PCI-like method of range property, there is no
> > need
> > start a new parser.
> >
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
>
> This patch, although it looks correct, breaks the RK3399-based
> systems, as they miss the (now required) 'device_type = "pci";'
> property.

Required since 1990 something...

> We can fix the in-tree DT, but that's not really an option
> if someone relies on the DT being provided by the firmware
> (I for one definitely do).

Perhaps time to pay attention to schema errors:

arch/arm64/boot/dts/rockchip/rk3399-sapphire-excavator.dt.yaml:
pcie@f8000000: 'device_type' is a required property

(I thought dtc would also catch this, but there we look for
device_type and then do PCI checks like node name. I guess we needed
to check for either device_type or the node name...)

> I came up with the following hack, which solves the issue for
> me. Definitely not my finest hour, but I really need this box
> to keep going. I will post a patch fixing the DT separately.
>
> Thanks,
>
>          M.
>
>  From ceef5fd9c4d2005eb577505c68868ebe527c098f Mon Sep 17 00:00:00 2001
>  From: Marc Zyngier <maz@xxxxxxxxxx>
> Date: Fri, 14 Aug 2020 19:10:12 +0100
> Subject: [PATCH] of: address: Workaround broken DTs missing the
> 'device_type =
>   "pci"' property
>
> Recent changes to the PCI bus parsing made it mandatory for
> device trees nodes describing a PCI controller to have the
> 'device_type = "pci"' property for the node to be matched.
>
> Although this is compliant with the specification, it breaks
> existing device-trees that have been working fine for years
> (the Rockchip rk3399-based systems being a prime example of
> such breakage).
>
> In order to paper over the blunder, let's also match nodes
> that have the "linux,pci-domain" property, as they are
> pretty likely to be PCI nodes. This fixes the issue for
> systems such as the above platforms.
>
> Fixes: 2f96593ecc37 ("of_address: Add bus type match for pci ranges
> parser")
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
>   drivers/of/address.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 590493e04b01..712e03781a2a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -134,9 +134,12 @@ static int of_bus_pci_match(struct device_node *np)
>          * "pciex" is PCI Express
>          * "vci" is for the /chaos bridge on 1st-gen PCI powermacs
>          * "ht" is hypertransport
> +        * "linux,pci-domain" is a workaround for broken device trees
> +        * lacking the required "device_type" property.

I would suggest looking for 'pci' or 'pcie' node name instead.

You should remove linux,pci-domain from rk3399 as it is pointless when
there's a single PCI host bridge.

The other option is fixup the live tree with of_add_property() in the
Rockchip PCI driver. Less likely to impact anyone else.

>          */
>         return of_node_is_type(np, "pci") || of_node_is_type(np, "pciex") ||
> -               of_node_is_type(np, "vci") || of_node_is_type(np, "ht");
> +               of_node_is_type(np, "vci") || of_node_is_type(np, "ht") ||
> +               of_find_property(np, "linux,pci-domain", NULL);
>   }
>
>   static void of_bus_pci_count_cells(struct device_node *np,
> --
> 2.27.0
>
>
> --
> Jazz is not dead. It just smells funny...