Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2] of: Fix of_empty_ranges_quirk()
@ 2019-09-07 16:15 marek.vasut
  2019-09-09 17:37 ` Rob Herring
  2019-09-13 15:29 ` Rob Herring
  0 siblings, 2 replies; 5+ messages in thread
From: marek.vasut @ 2019-09-07 16:15 UTC (permalink / raw)
  To: devicetree; +Cc: Marek Vasut, Rob Herring, Frank Rowand, linux-renesas-soc

From: Marek Vasut <marek.vasut+renesas@gmail.com>

The of_empty_ranges_quirk() returns a mix of boolean and signed integer
types, which cannot work well. Replace that with boolean only and fix
usage logic in of_translate_one() -- the check should trigger when the
ranges are NULL and the quirk is applicable on the hardware.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: linux-renesas-soc@vger.kernel.org
To: devicetree@vger.kernel.org
---
V2: - Rename of_empty_ranges_quirk() to of_missing_ranges_is_ok()
    - Move comment into the of_missing_ranges_is_ok() function
    - Make of_missing_ranges_is_ok() a bit more readable by adding
      a variable marking the quirk_state as initialized.
    - Reinstate check for !of_missing_ranges_is_ok() in of_translate_one()
---
 drivers/of/address.c | 47 ++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 978427a9d5e6..df82ef88823f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -473,21 +473,42 @@ static struct of_bus *of_match_bus(struct device_node *np)
 	return NULL;
 }
 
-static int of_empty_ranges_quirk(struct device_node *np)
+static bool of_missing_ranges_is_ok(struct device_node *np)
 {
+	/*
+	 * Normally, an absence of a "ranges" property means we are
+	 * crossing a non-translatable boundary, and thus the addresses
+	 * below the current cannot be converted to CPU physical ones.
+	 * Unfortunately, while this is very clear in the spec, it's not
+	 * what Apple understood, and they do have things like /uni-n or
+	 * /ht nodes with no "ranges" property and a lot of perfectly
+	 * useable mapped devices below them. Thus we treat the absence of
+	 * "ranges" as equivalent to an empty "ranges" property which means
+	 * a 1:1 translation at that level. It's up to the caller not to try
+	 * to translate addresses that aren't supposed to be translated in
+	 * the first place. --BenH.
+	 *
+	 * As far as we know, this damage only exists on Apple machines, so
+	 * This code is only enabled on powerpc.
+	 */
+
 	if (IS_ENABLED(CONFIG_PPC)) {
 		/* To save cycles, we cache the result for global "Mac" setting */
-		static int quirk_state = -1;
+		static int quirk_state_initialized;
+		static bool quirk_state;
 
 		/* PA-SEMI sdc DT bug */
 		if (of_device_is_compatible(np, "1682m-sdc"))
 			return true;
 
 		/* Make quirk cached */
-		if (quirk_state < 0)
+		if (!quirk_state_initialized) {
+			quirk_state_initialized = 1;
 			quirk_state =
 				of_machine_is_compatible("Power Macintosh") ||
 				of_machine_is_compatible("MacRISC");
+		}
+
 		return quirk_state;
 	}
 	return false;
@@ -502,25 +523,9 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
 	int rone;
 	u64 offset = OF_BAD_ADDR;
 
-	/*
-	 * Normally, an absence of a "ranges" property means we are
-	 * crossing a non-translatable boundary, and thus the addresses
-	 * below the current cannot be converted to CPU physical ones.
-	 * Unfortunately, while this is very clear in the spec, it's not
-	 * what Apple understood, and they do have things like /uni-n or
-	 * /ht nodes with no "ranges" property and a lot of perfectly
-	 * useable mapped devices below them. Thus we treat the absence of
-	 * "ranges" as equivalent to an empty "ranges" property which means
-	 * a 1:1 translation at that level. It's up to the caller not to try
-	 * to translate addresses that aren't supposed to be translated in
-	 * the first place. --BenH.
-	 *
-	 * As far as we know, this damage only exists on Apple machines, so
-	 * This code is only enabled on powerpc. --gcl
-	 */
 	ranges = of_get_property(parent, rprop, &rlen);
-	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
-		pr_debug("no ranges; cannot translate\n");
+	if (ranges == NULL && !of_missing_ranges_is_ok(parent)) {
+		pr_err("no ranges; cannot translate\n");
 		return 1;
 	}
 	if (ranges == NULL || rlen == 0) {
-- 
2.23.0.rc1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] of: Fix of_empty_ranges_quirk()
  2019-09-07 16:15 [PATCH V2] of: Fix of_empty_ranges_quirk() marek.vasut
@ 2019-09-09 17:37 ` Rob Herring
  2019-09-14 16:00   ` Marek Vasut
  2019-09-13 15:29 ` Rob Herring
  1 sibling, 1 reply; 5+ messages in thread
From: Rob Herring @ 2019-09-09 17:37 UTC (permalink / raw)
  To: Marek Vašut
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Sat, Sep 7, 2019 at 5:15 PM <marek.vasut@gmail.com> wrote:
>
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> types, which cannot work well. Replace that with boolean only and fix
> usage logic in of_translate_one() -- the check should trigger when the
> ranges are NULL and the quirk is applicable on the hardware.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: devicetree@vger.kernel.org
> ---
> V2: - Rename of_empty_ranges_quirk() to of_missing_ranges_is_ok()
>     - Move comment into the of_missing_ranges_is_ok() function
>     - Make of_missing_ranges_is_ok() a bit more readable by adding
>       a variable marking the quirk_state as initialized.
>     - Reinstate check for !of_missing_ranges_is_ok() in of_translate_one()
> ---
>  drivers/of/address.c | 47 ++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 978427a9d5e6..df82ef88823f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -473,21 +473,42 @@ static struct of_bus *of_match_bus(struct device_node *np)
>         return NULL;
>  }
>
> -static int of_empty_ranges_quirk(struct device_node *np)
> +static bool of_missing_ranges_is_ok(struct device_node *np)
>  {
> +       /*
> +        * Normally, an absence of a "ranges" property means we are
> +        * crossing a non-translatable boundary, and thus the addresses
> +        * below the current cannot be converted to CPU physical ones.
> +        * Unfortunately, while this is very clear in the spec, it's not
> +        * what Apple understood, and they do have things like /uni-n or
> +        * /ht nodes with no "ranges" property and a lot of perfectly
> +        * useable mapped devices below them. Thus we treat the absence of
> +        * "ranges" as equivalent to an empty "ranges" property which means
> +        * a 1:1 translation at that level. It's up to the caller not to try
> +        * to translate addresses that aren't supposed to be translated in
> +        * the first place. --BenH.
> +        *
> +        * As far as we know, this damage only exists on Apple machines, so
> +        * This code is only enabled on powerpc.

You dropped Grant's name on this. Wouldn't matter too much as we can
run 'git blame', but now we have another level to trace back thru.

> +        */
> +
>         if (IS_ENABLED(CONFIG_PPC)) {
>                 /* To save cycles, we cache the result for global "Mac" setting */
> -               static int quirk_state = -1;
> +               static int quirk_state_initialized;

This can be bool too.

> +               static bool quirk_state;
>
>                 /* PA-SEMI sdc DT bug */
>                 if (of_device_is_compatible(np, "1682m-sdc"))
>                         return true;
>
>                 /* Make quirk cached */
> -               if (quirk_state < 0)
> +               if (!quirk_state_initialized) {
> +                       quirk_state_initialized = 1;
>                         quirk_state =
>                                 of_machine_is_compatible("Power Macintosh") ||
>                                 of_machine_is_compatible("MacRISC");
> +               }
> +
>                 return quirk_state;
>         }
>         return false;
> @@ -502,25 +523,9 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>         int rone;
>         u64 offset = OF_BAD_ADDR;
>
> -       /*
> -        * Normally, an absence of a "ranges" property means we are
> -        * crossing a non-translatable boundary, and thus the addresses
> -        * below the current cannot be converted to CPU physical ones.
> -        * Unfortunately, while this is very clear in the spec, it's not
> -        * what Apple understood, and they do have things like /uni-n or
> -        * /ht nodes with no "ranges" property and a lot of perfectly
> -        * useable mapped devices below them. Thus we treat the absence of
> -        * "ranges" as equivalent to an empty "ranges" property which means
> -        * a 1:1 translation at that level. It's up to the caller not to try
> -        * to translate addresses that aren't supposed to be translated in
> -        * the first place. --BenH.
> -        *
> -        * As far as we know, this damage only exists on Apple machines, so
> -        * This code is only enabled on powerpc. --gcl
> -        */
>         ranges = of_get_property(parent, rprop, &rlen);
> -       if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> -               pr_debug("no ranges; cannot translate\n");
> +       if (ranges == NULL && !of_missing_ranges_is_ok(parent)) {
> +               pr_err("no ranges; cannot translate\n");
>                 return 1;
>         }
>         if (ranges == NULL || rlen == 0) {
> --
> 2.23.0.rc1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] of: Fix of_empty_ranges_quirk()
  2019-09-07 16:15 [PATCH V2] of: Fix of_empty_ranges_quirk() marek.vasut
  2019-09-09 17:37 ` Rob Herring
@ 2019-09-13 15:29 ` Rob Herring
  2019-09-14 16:05   ` Marek Vasut
  1 sibling, 1 reply; 5+ messages in thread
From: Rob Herring @ 2019-09-13 15:29 UTC (permalink / raw)
  To: Marek Vašut
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On Sat, Sep 7, 2019 at 5:15 PM <marek.vasut@gmail.com> wrote:
>
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>
> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
> types, which cannot work well. Replace that with boolean only and fix
> usage logic in of_translate_one() -- the check should trigger when the
> ranges are NULL and the quirk is applicable on the hardware.

Just moving to boolean has seemed like weak justification for this
churn, but now that I've seen your work on PCI dma-ranges it makes a
bit more sense.

We do have a problem that unlike 'ranges', 'dma-ranges' being missing
probably needs to be treated as 1:1 translation. I can't really
picture a case where dma-ranges would be used with a non-translatable
address. Perhaps a module with optional DMA and the DMA is not hooked
up. That could be expressed with dma-ranges with a 0 size or a
different compatible. So your v1 patch was perhaps correct change in
behavior, but only for dma-ranges. (I've written one that works in
both cases).

Rob

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] of: Fix of_empty_ranges_quirk()
  2019-09-09 17:37 ` Rob Herring
@ 2019-09-14 16:00   ` Marek Vasut
  0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2019-09-14 16:00 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On 9/9/19 7:37 PM, Rob Herring wrote:
[...]
>>  drivers/of/address.c | 47 ++++++++++++++++++++++++--------------------
>>  1 file changed, 26 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index 978427a9d5e6..df82ef88823f 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -473,21 +473,42 @@ static struct of_bus *of_match_bus(struct device_node *np)
>>         return NULL;
>>  }
>>
>> -static int of_empty_ranges_quirk(struct device_node *np)
>> +static bool of_missing_ranges_is_ok(struct device_node *np)
>>  {
>> +       /*
>> +        * Normally, an absence of a "ranges" property means we are
>> +        * crossing a non-translatable boundary, and thus the addresses
>> +        * below the current cannot be converted to CPU physical ones.
>> +        * Unfortunately, while this is very clear in the spec, it's not
>> +        * what Apple understood, and they do have things like /uni-n or
>> +        * /ht nodes with no "ranges" property and a lot of perfectly
>> +        * useable mapped devices below them. Thus we treat the absence of
>> +        * "ranges" as equivalent to an empty "ranges" property which means
>> +        * a 1:1 translation at that level. It's up to the caller not to try
>> +        * to translate addresses that aren't supposed to be translated in
>> +        * the first place. --BenH.
>> +        *
>> +        * As far as we know, this damage only exists on Apple machines, so
>> +        * This code is only enabled on powerpc.
> 
> You dropped Grant's name on this. Wouldn't matter too much as we can
> run 'git blame', but now we have another level to trace back thru.

I think that's what Frank suggested, since he reworded this message before.

>> +        */
>> +
>>         if (IS_ENABLED(CONFIG_PPC)) {
>>                 /* To save cycles, we cache the result for global "Mac" setting */
>> -               static int quirk_state = -1;
>> +               static int quirk_state_initialized;
> 
> This can be bool too.

Fine

[...]

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] of: Fix of_empty_ranges_quirk()
  2019-09-13 15:29 ` Rob Herring
@ 2019-09-14 16:05   ` Marek Vasut
  0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2019-09-14 16:05 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Marek Vasut, Frank Rowand,
	open list:MEDIA DRIVERS FOR RENESAS - FCP

On 9/13/19 5:29 PM, Rob Herring wrote:
>> The of_empty_ranges_quirk() returns a mix of boolean and signed integer
>> types, which cannot work well. Replace that with boolean only and fix
>> usage logic in of_translate_one() -- the check should trigger when the
>> ranges are NULL and the quirk is applicable on the hardware.
> 
> Just moving to boolean has seemed like weak justification for this
> churn, but now that I've seen your work on PCI dma-ranges it makes a
> bit more sense.
> 
> We do have a problem that unlike 'ranges', 'dma-ranges' being missing
> probably needs to be treated as 1:1 translation. I can't really
> picture a case where dma-ranges would be used with a non-translatable
> address. Perhaps a module with optional DMA and the DMA is not hooked
> up. That could be expressed with dma-ranges with a 0 size or a
> different compatible. So your v1 patch was perhaps correct change in
> behavior, but only for dma-ranges. (I've written one that works in
> both cases).

Is that posted somewhere ?

The controller node /soc/pcie@fe000000/ already has dma-ranges [1]
though, so I wonder if it's the PCIe translation patches which need to
be updated then ?

[1]
https://elixir.bootlin.com/linux/v5.3-rc8/source/arch/arm64/boot/dts/renesas/r8a7795.dtsi#L2653

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-07 16:15 [PATCH V2] of: Fix of_empty_ranges_quirk() marek.vasut
2019-09-09 17:37 ` Rob Herring
2019-09-14 16:00   ` Marek Vasut
2019-09-13 15:29 ` Rob Herring
2019-09-14 16:05   ` Marek Vasut

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox