* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
@ 2018-02-28 18:27 Martin Fuzzey
2018-03-01 7:26 ` Mario Six
0 siblings, 1 reply; 6+ messages in thread
From: Martin Fuzzey @ 2018-02-28 18:27 UTC (permalink / raw)
To: u-boot
Hi,
since this commit:
commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd
Author: Mario Six <mario.six@gdsys.cc>
Date: Mon Jan 15 11:07:45 2018 +0100
gpio: pca953x_gpio: Make live-tree compatible
I am getting the error message "__of_translate_address: Bad cell count
for pcal6524 at 1"
I don't think the patch
- addr = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), "reg", 0);
+ addr = dev_read_addr(dev);
is correct since it replaces a simple lookup of the value "reg" property
with an attempted translation to a CPU address (which fails for I2C)
This does not cause the driver probe to fail since dev_read_addr()
returns FDT_ADDR_T_NONE (== -1) and is followed by
if (addr == 0)
return -ENODEV;
info->addr = addr;
Although addr is stored in the driver data the only thing it is used for
is to build the bank name
snprintf(name, sizeof(name), "gpio@%x_", info->addr);
I'm not even sure that using the value of the "reg" property as before
is really a good idea - what happens if we have multiple chips with same
address on seperate busses?
Maybe it would be better to use the DT node name? But would that break
existing configurations by renaming the device?
Regards,
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
2018-02-28 18:27 [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible Martin Fuzzey
@ 2018-03-01 7:26 ` Mario Six
2018-03-01 8:01 ` Mario Six
0 siblings, 1 reply; 6+ messages in thread
From: Mario Six @ 2018-03-01 7:26 UTC (permalink / raw)
To: u-boot
(Adding Simon, because of DM)
Hi Martin,
On Wed, Feb 28, 2018 at 7:27 PM, Martin Fuzzey <mfuzzey@parkeon.com> wrote:
> Hi,
>
> since this commit:
>
> commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd
> Author: Mario Six <mario.six@gdsys.cc>
> Date: Mon Jan 15 11:07:45 2018 +0100
>
> gpio: pca953x_gpio: Make live-tree compatible
>
>
> I am getting the error message "__of_translate_address: Bad cell count for
> pcal6524 at 1"
>
> I don't think the patch
>
> - addr = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), "reg", 0);
> + addr = dev_read_addr(dev);
>
> is correct since it replaces a simple lookup of the value "reg" property
> with an attempted translation to a CPU address (which fails for I2C)
>
I see the issue on one of our boards as well, but the weird thing is that the
translation should not happen: the I2C bus has no ranges property, which
should, according to the DeviceTree specification, cause the translation
routine to not attempt a translation (because a missing ranges property implies
that the device's address space is not translatable). I'll take a look at the
code; maybe it's a Linux-specific exception that was imported with the code
that I was not aware of.
The dev_read_addr() is the go-to function for drivers to get device addresses
(and have all issues like translation and the like taken care of
automatically), so it would be nice to keep it in my opinion. Hence, fixing
the underlying issue is probably preferable.
> This does not cause the driver probe to fail since dev_read_addr() returns
> FDT_ADDR_T_NONE (== -1) and is followed by
>
> if (addr == 0)
> return -ENODEV;
>
> info->addr = addr;
>
> Although addr is stored in the driver data the only thing it is used for is
> to build the bank name
>
> snprintf(name, sizeof(name), "gpio@%x_", info->addr);
>
> I'm not even sure that using the value of the "reg" property as before is
> really a good idea - what happens if we have multiple chips with same
> address on seperate busses?
>
We have that exact case on some of our boards. It works, but it's pretty
annoying if you want to use the gpio command: When setting values of GPIOs, it
only ever affects the first device the command comes across.
I have a potential solution for that, see below.
> Maybe it would be better to use the DT node name? But would that break
> existing configurations by renaming the device?
I have a patch I use on our boards that looks for a "label" string property in
the DT, which is then used for the bank name if it's there, and falls back to
the "gpio@%x_" in the case it's not there. If you think it's useful, I could
send the patch to the mailing list?
>
>
> Regards,
>
> Martin
>
Best regards,
Mario
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
2018-03-01 7:26 ` Mario Six
@ 2018-03-01 8:01 ` Mario Six
2018-03-01 10:07 ` Martin Fuzzey
0 siblings, 1 reply; 6+ messages in thread
From: Mario Six @ 2018-03-01 8:01 UTC (permalink / raw)
To: u-boot
On Thu, Mar 1, 2018 at 8:26 AM, Mario Six <mario.six@gdsys.cc> wrote:
> (Adding Simon, because of DM)
>
> Hi Martin,
>
> On Wed, Feb 28, 2018 at 7:27 PM, Martin Fuzzey <mfuzzey@parkeon.com> wrote:
>> Hi,
>>
>> since this commit:
>>
>> commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd
>> Author: Mario Six <mario.six@gdsys.cc>
>> Date: Mon Jan 15 11:07:45 2018 +0100
>>
>> gpio: pca953x_gpio: Make live-tree compatible
>>
>>
>> I am getting the error message "__of_translate_address: Bad cell count for
>> pcal6524 at 1"
>>
>> I don't think the patch
>>
>> - addr = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), "reg", 0);
>> + addr = dev_read_addr(dev);
>>
>> is correct since it replaces a simple lookup of the value "reg" property
>> with an attempted translation to a CPU address (which fails for I2C)
>>
>
> I see the issue on one of our boards as well, but the weird thing is that the
> translation should not happen: the I2C bus has no ranges property, which
> should, according to the DeviceTree specification, cause the translation
> routine to not attempt a translation (because a missing ranges property implies
> that the device's address space is not translatable). I'll take a look at the
> code; maybe it's a Linux-specific exception that was imported with the code
> that I was not aware of.
>
Looks like that's it (from drivers/core/of_addr.c):
* Note: We consider that crossing any level with #size-cells == 0 to mean
* that translation is impossible (that is we are not dealing with a value
* that can be mapped to a cpu physical address). This is not really specified
* that way, but this is traditionally the way IBM at least do things
So, of_translate_address fails by design if #size-cells == 0.
I propose the following patch, which would prevent translation in this case in
ofnode_get_addr_index:
---- >8 ----
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 98f4b539ea..b2f6ffe385 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -200,13 +200,16 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
uint flags;
u64 size;
int na;
+ int ns;
prop_val = of_get_address(ofnode_to_np(node), index, &size,
&flags);
if (!prop_val)
return FDT_ADDR_T_NONE;
- if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
+ ns = of_n_size_cells(ofnode_to_np(node));
+
+ if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
return
of_translate_address(ofnode_to_np(node), prop_val);
} else {
na = of_n_addr_cells(ofnode_to_np(node));
---- >8 ----
In other words:
#size-cells == 0 -> No translation, return the plain reg value.
#size-cells != 0 -> Do proper translation.
This fixes the issue on our board.
> The dev_read_addr() is the go-to function for drivers to get device addresses
> (and have all issues like translation and the like taken care of
> automatically), so it would be nice to keep it in my opinion. Hence, fixing
> the underlying issue is probably preferable.
>
>> This does not cause the driver probe to fail since dev_read_addr() returns
>> FDT_ADDR_T_NONE (== -1) and is followed by
>>
>> if (addr == 0)
>> return -ENODEV;
>>
>> info->addr = addr;
>>
>> Although addr is stored in the driver data the only thing it is used for is
>> to build the bank name
>>
>> snprintf(name, sizeof(name), "gpio@%x_", info->addr);
>>
>> I'm not even sure that using the value of the "reg" property as before is
>> really a good idea - what happens if we have multiple chips with same
>> address on seperate busses?
>>
>
> We have that exact case on some of our boards. It works, but it's pretty
> annoying if you want to use the gpio command: When setting values of GPIOs, it
> only ever affects the first device the command comes across.
>
> I have a potential solution for that, see below.
>
>> Maybe it would be better to use the DT node name? But would that break
>> existing configurations by renaming the device?
>
> I have a patch I use on our boards that looks for a "label" string property in
> the DT, which is then used for the bank name if it's there, and falls back to
> the "gpio@%x_" in the case it's not there. If you think it's useful, I could
> send the patch to the mailing list?
>
>>
>>
>> Regards,
>>
>> Martin
>>
>
> Best regards,
>
> Mario
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
2018-03-01 8:01 ` Mario Six
@ 2018-03-01 10:07 ` Martin Fuzzey
2018-03-01 13:42 ` Mario Six
0 siblings, 1 reply; 6+ messages in thread
From: Martin Fuzzey @ 2018-03-01 10:07 UTC (permalink / raw)
To: u-boot
Hi Mario,
thank you for your very quick reply.
On 01/03/18 09:01, Mario Six wrote:
>
> Looks like that's it (from drivers/core/of_addr.c):
>
> * Note: We consider that crossing any level with #size-cells == 0 to mean
> * that translation is impossible (that is we are not dealing with a value
> * that can be mapped to a cpu physical address). This is not really specified
> * that way, but this is traditionally the way IBM at least do things
>
> So, of_translate_address fails by design if #size-cells == 0.
>
> I propose the following patch, which would prevent translation in this case in
> ofnode_get_addr_index:
>
> ---- >8 ----
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 98f4b539ea..b2f6ffe385 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -200,13 +200,16 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int index)
> uint flags;
> u64 size;
> int na;
> + int ns;
>
> prop_val = of_get_address(ofnode_to_np(node), index, &size,
> &flags);
> if (!prop_val)
> return FDT_ADDR_T_NONE;
>
> - if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
> + ns = of_n_size_cells(ofnode_to_np(node));
> +
> + if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
> return
> of_translate_address(ofnode_to_np(node), prop_val);
> } else {
> na = of_n_addr_cells(ofnode_to_np(node));
> ---- >8 ----
>
> In other words:
> #size-cells == 0 -> No translation, return the plain reg value.
> #size-cells != 0 -> Do proper translation.
>
> This fixes the issue on our board.
That isn't enough for the non live-tree case.
I've added this too:
diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
index 3847dd8..9a3b4c3 100644
--- a/drivers/core/fdtaddr.c
+++ b/drivers/core/fdtaddr.c
@@ -49,12 +49,17 @@ fdt_addr_t devfdt_get_addr_index(struct udevice
*dev, int index)
reg += index * (na + ns);
- /*
- * Use the full-fledged translate function for complex
- * bus setups.
- */
- addr = fdt_translate_address((void *)gd->fdt_blob,
- dev_of_offset(dev), reg);
+ if (ns) {
+ /*
+ * Use the full-fledged translate function for
complex
+ * bus setups.
+ */
+ addr = fdt_translate_address((void *)gd->fdt_blob,
+ dev_of_offset(dev), reg);
+ } else {
+ /* Non translatable if #size-cells == 0 */
+ addr = fdt_read_number(reg, na);
+ }
} else {
/*
* Use the "simple" translate function for less complex
> I have a patch I use on our boards that looks for a "label" string
> property in
>> the DT, which is then used for the bank name if it's there, and falls back to
>> the "gpio@%x_" in the case it's not there. If you think it's useful, I could
>> send the patch to the mailing list?
Yes please.
Regards,
Martin
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
2018-03-01 10:07 ` Martin Fuzzey
@ 2018-03-01 13:42 ` Mario Six
2018-03-01 16:41 ` Martin Fuzzey
0 siblings, 1 reply; 6+ messages in thread
From: Mario Six @ 2018-03-01 13:42 UTC (permalink / raw)
To: u-boot
Hi Martin,
On Thu, Mar 1, 2018 at 11:07 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote:
> Hi Mario,
>
> thank you for your very quick reply.
>
>
> On 01/03/18 09:01, Mario Six wrote:
>>
>>
>> Looks like that's it (from drivers/core/of_addr.c):
>>
>> * Note: We consider that crossing any level with #size-cells == 0 to
>> mean
>> * that translation is impossible (that is we are not dealing with a
>> value
>> * that can be mapped to a cpu physical address). This is not really
>> specified
>> * that way, but this is traditionally the way IBM at least do things
>>
>> So, of_translate_address fails by design if #size-cells == 0.
>>
>> I propose the following patch, which would prevent translation in this
>> case in
>> ofnode_get_addr_index:
>>
>> ---- >8 ----
>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
>> index 98f4b539ea..b2f6ffe385 100644
>> --- a/drivers/core/ofnode.c
>> +++ b/drivers/core/ofnode.c
>> @@ -200,13 +200,16 @@ fdt_addr_t ofnode_get_addr_index(ofnode node, int
>> index)
>> uint flags;
>> u64 size;
>> int na;
>> + int ns;
>>
>> prop_val = of_get_address(ofnode_to_np(node), index,
>> &size,
>> &flags);
>> if (!prop_val)
>> return FDT_ADDR_T_NONE;
>>
>> - if (IS_ENABLED(CONFIG_OF_TRANSLATE)) {
>> + ns = of_n_size_cells(ofnode_to_np(node));
>> +
>> + if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
>> return
>> of_translate_address(ofnode_to_np(node), prop_val);
>> } else {
>> na = of_n_addr_cells(ofnode_to_np(node));
>> ---- >8 ----
>>
>> In other words:
>> #size-cells == 0 -> No translation, return the plain reg value.
>> #size-cells != 0 -> Do proper translation.
>>
>> This fixes the issue on our board.
>
>
> That isn't enough for the non live-tree case.
>
> I've added this too:
>
> diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c
> index 3847dd8..9a3b4c3 100644
> --- a/drivers/core/fdtaddr.c
> +++ b/drivers/core/fdtaddr.c
> @@ -49,12 +49,17 @@ fdt_addr_t devfdt_get_addr_index(struct udevice *dev,
> int index)
>
> reg += index * (na + ns);
>
> - /*
> - * Use the full-fledged translate function for complex
> - * bus setups.
> - */
> - addr = fdt_translate_address((void *)gd->fdt_blob,
> - dev_of_offset(dev), reg);
> + if (ns) {
> + /*
> + * Use the full-fledged translate function for
> complex
> + * bus setups.
> + */
> + addr = fdt_translate_address((void *)gd->fdt_blob,
> + dev_of_offset(dev), reg);
> + } else {
> + /* Non translatable if #size-cells == 0 */
> + addr = fdt_read_number(reg, na);
> + }
> } else {
> /*
> * Use the "simple" translate function for less complex
>
Ah, yes, indeed, thanks for catching that. The enhanced patch still works in
the live-tree case, so, can I go ahead and add your Signed-off-by to the patch
and send it?
Thanks for testing, and sorry for the inconvenience.
>> I have a patch I use on our boards that looks for a "label" string
>> property in
>>>
>>> the DT, which is then used for the bank name if it's there, and falls
>>> back to
>>> the "gpio@%x_" in the case it's not there. If you think it's useful, I
>>> could
>>> send the patch to the mailing list?
>
>
> Yes please.
>
OK, I'll send it in a few minutes.
>
> Regards,
>
> Martin
>
Best regards,
Mario
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible
2018-03-01 13:42 ` Mario Six
@ 2018-03-01 16:41 ` Martin Fuzzey
0 siblings, 0 replies; 6+ messages in thread
From: Martin Fuzzey @ 2018-03-01 16:41 UTC (permalink / raw)
To: u-boot
Hi Mario,
On 01/03/18 14:42, Mario Six wrote:
> Ah, yes, indeed, thanks for catching that. The enhanced patch still
> works in
> the live-tree case, so, can I go ahead and add your Signed-off-by to the patch
> and send it?
>
> Thanks for testing, and sorry for the inconvenience.
Yes sure,
Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Regards,
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-01 16:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 18:27 [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible Martin Fuzzey
2018-03-01 7:26 ` Mario Six
2018-03-01 8:01 ` Mario Six
2018-03-01 10:07 ` Martin Fuzzey
2018-03-01 13:42 ` Mario Six
2018-03-01 16:41 ` Martin Fuzzey
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.