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 compatibleI am getting the error message "__of_translate_address: Bad cell count for pcal6524@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