diff mbox series

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

Message ID 20200728153708.1296374-2-jiaxun.yang@flygoat.com
State Not Applicable, archived
Headers show
Series MIPS: Loongson64: Process ISA Node in DeviceTree | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Jiaxun Yang July 28, 2020, 3:36 p.m. UTC 
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@flygoat.com>
Reviewed-by: Rob Herring <robh@kernel.org>

--
v2: Drop useless check, fix some na for bus_addr
	add define of of_range_parser_init according to
	Rob's suggestion.
v3: Abstract out has_flags. simplify define.
---
 drivers/of/address.c       | 29 +++++++++++++++++------------
 include/linux/of_address.h |  4 ++++
 2 files changed, 21 insertions(+), 12 deletions(-)

Comments

Marc Zyngier Aug. 14, 2020, 6:21 p.m. UTC | #1 
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@flygoat.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

This patch, although it looks correct, breaks the RK3399-based
systems, as they miss the (now required) 'device_type = "pci";'
property.

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).

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@kernel.org>
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@kernel.org>
---
  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.
  	 */
  	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,
Rob Herring Aug. 14, 2020, 10:51 p.m. UTC | #2 
On Fri, Aug 14, 2020 at 12:21 PM Marc Zyngier <maz@kernel.org> 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@flygoat.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> 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@kernel.org>
> 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@kernel.org>
> ---
>   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...
Marc Zyngier Aug. 15, 2020, 11:22 a.m. UTC | #3 
On 2020-08-14 23:51, Rob Herring wrote:
> On Fri, Aug 14, 2020 at 12:21 PM Marc Zyngier <maz@kernel.org> 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@flygoat.com>
>> > Reviewed-by: Rob Herring <robh@kernel.org>
>> 
>> 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...

And yet... The fact that it has been broken from day-1 (3.5 years
ago) shows how good we are at enforcing those requirements.

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

As long as this isn't part of the normal build flow, these errors
will get ignored. I don't even know how to trigger this validation,
TBH.

> (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...)

That would be much better. At least this *does* run at build time.

> 
>> 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@kernel.org>
>> 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@kernel.org>
>> ---
>>   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.

Indeed. I was clutching at straws here.

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

That's actually a much better solution, thanks for pointing this out.
At least I can shout about broken DTs while fixing it up, and the
new fix is fairly neat.

I'll post new patches shortly.

Thanks,

         M.
diff mbox series

Patch

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 8eea3f6e29a4..813936d419ad 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -49,6 +49,7 @@  struct of_bus {
 	u64		(*map)(__be32 *addr, const __be32 *range,
 				int na, int ns, int pna);
 	int		(*translate)(__be32 *addr, u64 offset, int na);
+	bool	has_flags;
 	unsigned int	(*get_flags)(const __be32 *addr);
 };
 
@@ -364,6 +365,7 @@  static struct of_bus of_busses[] = {
 		.count_cells = of_bus_pci_count_cells,
 		.map = of_bus_pci_map,
 		.translate = of_bus_pci_translate,
+		.has_flags = true,
 		.get_flags = of_bus_pci_get_flags,
 	},
 #endif /* CONFIG_PCI */
@@ -375,6 +377,7 @@  static struct of_bus of_busses[] = {
 		.count_cells = of_bus_isa_count_cells,
 		.map = of_bus_isa_map,
 		.translate = of_bus_isa_translate,
+		.has_flags = true,
 		.get_flags = of_bus_isa_get_flags,
 	},
 	/* Default */
@@ -698,9 +701,10 @@  static int parser_init(struct of_pci_range_parser *parser,
 
 	parser->node = node;
 	parser->pna = of_n_addr_cells(node);
-	parser->na = of_bus_n_addr_cells(node);
-	parser->ns = of_bus_n_size_cells(node);
 	parser->dma = !strcmp(name, "dma-ranges");
+	parser->bus = of_match_bus(node);
+
+	parser->bus->count_cells(parser->node, &parser->na, &parser->ns);
 
 	parser->range = of_get_property(node, name, &rlen);
 	if (parser->range == NULL)
@@ -732,6 +736,7 @@  struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 	int na = parser->na;
 	int ns = parser->ns;
 	int np = parser->pna + na + ns;
+	int busflag_na = 0;
 
 	if (!range)
 		return NULL;
@@ -739,12 +744,13 @@  struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 	if (!parser->range || parser->range + np > parser->end)
 		return NULL;
 
-	if (parser->na == 3)
-		range->flags = of_bus_pci_get_flags(parser->range);
-	else
-		range->flags = 0;
+	range->flags = parser->bus->get_flags(parser->range);
+
+	/* A extra cell for resource flags */
+	if (parser->bus->has_flags)
+		busflag_na = 1;
 
-	range->pci_addr = of_read_number(parser->range, na);
+	range->bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
 
 	if (parser->dma)
 		range->cpu_addr = of_translate_dma_address(parser->node,
@@ -759,11 +765,10 @@  struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 	/* Now consume following elements while they are contiguous */
 	while (parser->range + np <= parser->end) {
 		u32 flags = 0;
-		u64 pci_addr, cpu_addr, size;
+		u64 bus_addr, cpu_addr, size;
 
-		if (parser->na == 3)
-			flags = of_bus_pci_get_flags(parser->range);
-		pci_addr = of_read_number(parser->range, na);
+		flags = parser->bus->get_flags(parser->range);
+		bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
 		if (parser->dma)
 			cpu_addr = of_translate_dma_address(parser->node,
 					parser->range + na);
@@ -774,7 +779,7 @@  struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
 
 		if (flags != range->flags)
 			break;
-		if (pci_addr != range->pci_addr + range->size ||
+		if (bus_addr != range->bus_addr + range->size ||
 		    cpu_addr != range->cpu_addr + range->size)
 			break;
 
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 763022ed3456..88bc943405cd 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -6,8 +6,11 @@ 
 #include <linux/of.h>
 #include <linux/io.h>
 
+struct of_bus;
+
 struct of_pci_range_parser {
 	struct device_node *node;
+	struct of_bus *bus;
 	const __be32 *range;
 	const __be32 *end;
 	int na;
@@ -119,6 +122,7 @@  static inline void __iomem *of_iomap(struct device_node *device, int index)
 	return NULL;
 }
 #endif
+#define of_range_parser_init of_pci_range_parser_init
 
 #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_PCI)
 extern const __be32 *of_get_pci_address(struct device_node *dev, int bar_no,