From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Six Date: Thu, 1 Mar 2018 09:01:01 +0100 Subject: [U-Boot] Regression: gpio: pca953x_gpio: Make live-tree compatible In-Reply-To: References: Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Mar 1, 2018 at 8:26 AM, Mario Six wrote: > (Adding Simon, because of DM) > > Hi Martin, > > On Wed, Feb 28, 2018 at 7:27 PM, Martin Fuzzey wrote: >> Hi, >> >> since this commit: >> >> commit f62ca2cd2ab8171f603960da9203ceb6ee8a1efd >> Author: Mario Six >> 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